Skip to content

Implement JsonSerde for all datatypes #53

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.

Sign up for GitHub

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

Jump to bottom
Merged
luoyuxia merged 1 commit into from
Dec 5, 2025
Merged

Implement JsonSerde for all datatypes #53

luoyuxia merged 1 commit into from
Dec 5, 2025

Conversation

@pavlospt
Copy link
Contributor

@pavlospt pavlospt commented Oct 26, 2025 •
edited
Loading

Resolves issue #39: Implement JsonSerde for all datatypes

This PR completes the implementation of JsonSerde for all DataType variants, including complex types like Decimal, Time, Timestamp, TimestampLTz, Array, Map, and Row.

Changes include:

  • Fixed typo in FIELD_NAME_SCALE constant
  • Implemented JsonSerde for DataField
  • Added serialization/deserialization for all DataType variants
  • Added comprehensive tests

All tests pass.

Fixes #39

@pavlospt pavlospt force-pushed the feat/implement-jsonsserde-for-all-datatypes branch from e059a0e to 219c595 Compare October 26, 2025 19:18
@luoyuxia luoyuxia requested a review from Copilot November 24, 2025 04:14
Copilot AI reviewed Nov 24, 2025
View reviewed changes
Copy link

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 implements JSON serialization and deserialization for all DataType variants in the Fluss metadata system, completing issue #39. It adds support for complex types (Decimal, Time, Timestamp variants, Array, Map, Row) that were previously marked with todo!() placeholders.

Key changes:

  • Fixed typo in constant name (FILED_NAME_SCALE → FIELD_NAME_SCALE)
  • Implemented JsonSerde for all DataType variants including nested/complex types
  • Implemented JsonSerde for DataField to support Row type serialization
  • Improved error message consistency ("should be" → "must be")
  • Added comprehensive test coverage for all data types

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

@binary-signal
Copy link
Contributor

binary-signal commented Nov 24, 2025

@pavlospt fluss doesn't support yet Array, Map types. it's on the roadmap for the next release 0.9.0

This was referenced Nov 24, 2025
Merged
Merged
@luoyuxia
Copy link
Contributor

luoyuxia commented Nov 25, 2025

@pavlospt fluss doesn't support yet Array, Map types. it's on the roadmap for the next release 0.9.0

Although fluss doesn't support it yet, we can define them in spec just like what we do in fluss-java, where we define ArrayType, MapType

luoyuxia
luoyuxia reviewed Nov 29, 2025
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

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

@pavlospt Thanks for the pr. Left minor comments.

@pavlospt pavlospt force-pushed the feat/implement-jsonsserde-for-all-datatypes branch 2 times, most recently from 376c459 to b5f37f9 Compare November 30, 2025 21:22
@luoyuxia
Copy link
Contributor

luoyuxia commented Dec 3, 2025

@pavlospt Hi, ci fails

@pavlospt
Copy link
Contributor Author

pavlospt commented Dec 3, 2025

@pavlospt Hi, ci fails

It needs to go through manual approval to run the CI every time so I will fix it now :D

@pavlospt
Implement JsonSerde for all datatypes
49d0fad 
- Fix typo in FIELD_NAME_SCALE constant
- Implement JsonSerde for DataField
- Complete serialize_json and deserialize_json for all DataType variants
- Add comprehensive test for DataType json serialization/deserialization

Resolves issue apache#39

Amp-Thread-ID: https://ampcode.com/threads/T-12d3046f-c788-4b3e-b1ae-6830b05aa926
Co-authored-by: Amp <[email protected]>

Update crates/fluss/src/metadata/json_serde.rs

Co-authored-by: Copilot <[email protected]>

Update crates/fluss/src/metadata/json_serde.rs

Co-authored-by: Copilot <[email protected]>

Update crates/fluss/src/metadata/json_serde.rs

Co-authored-by: Copilot <[email protected]>

Update crates/fluss/src/metadata/json_serde.rs

Co-authored-by: Copilot <[email protected]>
@pavlospt pavlospt force-pushed the feat/implement-jsonsserde-for-all-datatypes branch from b5f37f9 to 49d0fad Compare December 3, 2025 22:19
luoyuxia
luoyuxia approved these changes Dec 5, 2025
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

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

@pavlospt Thank you very much. LGTM

@luoyuxia luoyuxia merged commit 12d464e into apache:main Dec 5, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@luoyuxia luoyuxia luoyuxia approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement JsonSerde for all datatypes

3 participants

@pavlospt @binary-signal @luoyuxia