-
Notifications
You must be signed in to change notification settings - Fork 425
refactor: create JsonValue for json value
#7214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors JSON value representation by introducing a dedicated JsonValue struct instead of using the generic Value type. The new implementation uses BTreeMap for JSON objects to maintain sorted field ordering, simplifying code when working with different JSON variants.
- Introduces new
JsonValue,JsonValueRef,JsonVariant, andJsonVariantReftypes with sorted field support - Updates JSON encoding/decoding logic to use the new types throughout the codebase
- Enforces homogeneous type requirements for JSON array items (all items must have the same type)
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/datatypes/src/json/value.rs | New file defining JsonValue and related types with sorted JSON object fields using BTreeMap |
| src/datatypes/src/value.rs | Updated Value enum to use Box<JsonValue> instead of Box<Value> for JSON variant |
| src/datatypes/src/json.rs | Refactored JSON encoding/decoding to use JsonValue types and enforce array type homogeneity |
| src/datatypes/src/types/json_type.rs | Added helper methods and extracted plain_json_struct_type function |
| src/datatypes/src/vectors/json/builder.rs | Updated JSON vector builder to work with new JsonValueRef type |
| src/api/src/helper.rs | Added encode_json_value and decode_json_value functions for protobuf conversion |
| src/frontend/src/limiter.rs | Updated size calculation logic for new JSON value structure |
| src/partition/src/collider.rs | Added clippy allow annotation for mutable key type usage |
| Cargo.toml / Cargo.lock | Updated greptime-proto dependency to support new JSON value representation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sunng87
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to double-check with this JsonValue, are we still using Arrow's Struct for json object internally? Because we need that for flattening nested data in parquet, which is of critical importance to ensure a columnar format for json
@sunng87 Physically the |
Signed-off-by: luofucong <[email protected]>
5fbd23c to
737e069
Compare
|
@MichaelScofield I've opened a new pull request, #7217, to work on those changes. Once the pull request is ready, I'll request review from you. |
737e069 to
3ce00ee
Compare
|
@copilot review |
|
@MichaelScofield I've opened a new pull request, #7218, to work on those changes. Once the pull request is ready, I'll request review from you. |
Signed-off-by: luofucong <[email protected]>
3ce00ee to
6305b29
Compare
Signed-off-by: luofucong <[email protected]>
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
Make json value represented as struct
JsonValue, instead ofValue. New json value has its fields sorted, which is more convenient for further use. And lesser verbose codes when dealing with different json variants.related proto change: GreptimeTeam/greptime-proto#289
PR Checklist
Please convert it to a draft if some of the following conditions are not met.