Skip to content

Conversation

@jackyyyyyssss
Copy link
Contributor

@jackyyyyyssss jackyyyyyssss commented Jul 1, 2024

https://redis.io/docs/latest/commands/json.resp/
Local testing of multi-layer nested JSON results 6666 kvrocks and 6388 redis stack
image

image
image
image

@jackyyyyyssss jackyyyyyssss marked this pull request as draft July 1, 2024 11:29
@jackyyyyyssss jackyyyyyssss marked this pull request as ready for review July 1, 2024 11:30
src/types/json.h Outdated
Comment on lines 46 to 53
struct ArrayOrObjectPlaceHolder {};
using VariantType = std::variant<std::string, int64_t, ArrayOrObjectPlaceHolder, bool>;
template <typename T>
struct JsonResp {
std::vector<std::unique_ptr<JsonResp<VariantType>>> children;
T value;
explicit JsonResp(T value) : value(std::move(value)) {}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove all of these types and just dump the RESP string directly.

Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

@jackyyyyyssss jackyyyyyssss requested a review from PragmaTwice July 5, 2024 02:02
@git-hulk git-hulk requested a review from PragmaTwice July 15, 2024 03:16
src/types/json.h Outdated
Comment on lines 674 to 676
if (path != "$") {
return redis::MultiLen(query_count) + json_resps;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems not consistent with RedisJSON here.

json.resp a $ and json.resp a have different behavior.

Copy link
Member

@PragmaTwice PragmaTwice Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can move redis::MultiLen(query_count) here to CommandJsonResp::Parse/Execute.

and here we can return vector<string>.

Copy link
Contributor Author

@jackyyyyyssss jackyyyyyssss Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reminder. It has not been modified yet. make the PR draft first

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been modified, but it is a bit unsupported. jsoncons does not support queries without $, Redis 7.0 supports it and will remove the outermost array

@jackyyyyyssss jackyyyyyssss marked this pull request as draft July 22, 2024 09:07
@jackyyyyyssss jackyyyyyssss marked this pull request as ready for review July 23, 2024 03:10
@git-hulk git-hulk requested review from PragmaTwice and git-hulk July 23, 2024 03:50
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants