Skip to content

Conversation

@MichaelScofield
Copy link
Collaborator

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?

instead of being backed by ConcreteDataType directly, which makes json value verbose to operate its type

also the new json type is precisely corresponding to the json value, making codes lesser error-prone

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

@github-actions github-actions bot added size/M docs-not-required This change does not impact docs. labels Nov 18, 2025
Copilot finished reviewing on behalf of MichaelScofield November 18, 2025 03:32
Copy link
Contributor

Copilot AI left a 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 the JSON value implementation to use a dedicated JsonNativeType enum instead of being directly backed by ConcreteDataType. This makes the JSON type system more precise and reduces verbosity when operating on JSON types.

Key changes:

  • Introduced new JsonNativeType enum with variants for Null, Bool, Number, String, Array, and Object
  • Implemented bidirectional conversions between JsonNativeType and ConcreteDataType
  • Updated all JSON encoding/decoding logic to use JsonNativeType internally

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/datatypes/src/types/json_type.rs Introduces JsonNativeType enum and refactors JsonType to use it; simplifies merge logic
src/datatypes/src/json/value.rs Updates JsonVariant and JsonVariantRef to work with JsonNativeType
src/datatypes/src/json.rs Refactors encoding functions to accept JsonNativeType instead of ConcreteDataType
src/datatypes/src/types/struct_type.rs Adds #[expect(unused)] attribute to metadata() method
src/datatypes/src/data_type.rs Updates json_native_datatype() to use new conversion
src/api/src/helper.rs Adjusts conversion logic for JsonFormat::Native
Comments suppressed due to low confidence (1)

src/datatypes/src/json/value.rs:1

  • Unnecessary clone operation. The native_type() method now returns &JsonNativeType (a reference), so calling .clone() on each array item creates unnecessary allocations. Consider refactoring to avoid cloning in the loop.
// Copyright 2023 Greptime Team

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MichaelScofield MichaelScofield force-pushed the refactor/make-json-value-use-json-type branch 5 times, most recently from 5bf0f14 to 087dd64 Compare November 18, 2025 10:22
@MichaelScofield MichaelScofield marked this pull request as ready for review November 18, 2025 10:55
@MichaelScofield MichaelScofield requested a review from a team as a code owner November 18, 2025 10:55
Copilot finished reviewing on behalf of MichaelScofield November 18, 2025 10:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MichaelScofield MichaelScofield force-pushed the refactor/make-json-value-use-json-type branch from 087dd64 to 641659b Compare November 21, 2025 11:06
@MichaelScofield MichaelScofield added this pull request to the merge queue Nov 24, 2025
Merged via the queue into main with commit ff99bce Nov 24, 2025
43 checks passed
@MichaelScofield MichaelScofield deleted the refactor/make-json-value-use-json-type branch November 24, 2025 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required This change does not impact docs. size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants