Skip to content

feat: support decimal literal and refactor transform utilities #238

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
merged 7 commits into from
Oct 19, 2025
Merged

feat: support decimal literal and refactor transform utilities #238

merged 7 commits into from
Oct 19, 2025
+1,548 −557

Conversation

@zhjwpku
Copy link
Collaborator

@zhjwpku zhjwpku commented Sep 19, 2025 •
edited
Loading

This PR respects Appendix B: 32-bit Hash Requirements and also closes #164.

@zhjwpku zhjwpku requested a review from wgtmac September 22, 2025 05:01
@HeartLinked
Copy link
Contributor

HeartLinked commented Sep 22, 2025 •
edited
Loading

Thanks for your contribution! I noticed part of the code logic in your PR overlaps with mine (#185). My PR has been in review for a long time, and it’s now almost ready for merge. It shouldn’t take long to finalize.​
To avoid unnecessary code conflicts, could you wait until my PR is merged? Then you can rebase your code on the merged version and only focus on adding the new Literal support for decimal. This way, your core contribution is preserved, and we reduce maintenance costs from duplicate code. Thanks!

@zhjwpku
Copy link
Collaborator Author

zhjwpku commented Sep 22, 2025

Thanks for your contribution! I noticed part of the code logic in your PR overlaps with mine (#185). My PR has been in review for a long time, and it’s now almost ready for merge. It shouldn’t take long to finalize.​To avoid unnecessary code conflicts and make our collaboration more efficient, could you wait until my PR is merged? Then you can rebase your code on the merged version and only focus on adding the new Literal support for decimal. This way, your core contribution is preserved, and we reduce maintenance costs from duplicate code. Thanks!

Sure thing, I will take a look of your PR, thanks.

@HeartLinked HeartLinked mentioned this pull request Sep 23, 2025
Merged
HuaHuaY
HuaHuaY reviewed Sep 23, 2025
View reviewed changes
@zhjwpku zhjwpku mentioned this pull request Oct 9, 2025
Merged
@zhjwpku zhjwpku force-pushed the implement_literal_ser_de branch from d2af530 to fe199ba Compare October 12, 2025 14:46
@zhjwpku zhjwpku changed the title feat: Literal support decimal & Literal serde feat: Literal support decimal & some refactor Oct 12, 2025
@zhjwpku zhjwpku force-pushed the implement_literal_ser_de branch from fe199ba to 9b4b7fa Compare October 12, 2025 15:10
wgtmac
wgtmac reviewed Oct 14, 2025
View reviewed changes
src/iceberg/test/bucket_util_test.cc

// timestamp
// 2017-11-16T22:31:08 in microseconds
std::chrono::system_clock::time_point tp =
Copy link
Member

@wgtmac wgtmac Oct 14, 2025

Choose a reason for hiding this comment

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

Perhaps we can add some utility functions in the temporal_util.h to create date/time/timestamp from structural inputs.

Copy link
Collaborator Author

@zhjwpku zhjwpku Oct 16, 2025

Choose a reason for hiding this comment

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

Agreed, do you mind if I add these in a separate PR?

Copy link
Member

@wgtmac wgtmac Oct 17, 2025

Choose a reason for hiding this comment

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

No problem!

wgtmac
wgtmac requested changes Oct 14, 2025
View reviewed changes
@zhjwpku zhjwpku force-pushed the implement_literal_ser_de branch from 9b4b7fa to 5e9bb1b Compare October 16, 2025 00:47
HuaHuaY
HuaHuaY reviewed Oct 16, 2025
View reviewed changes
@zhjwpku zhjwpku force-pushed the implement_literal_ser_de branch from a74dfdf to fbc9d10 Compare October 16, 2025 16:03
@zhjwpku zhjwpku requested review from HuaHuaY and wgtmac October 17, 2025 00:01
wgtmac
wgtmac reviewed Oct 17, 2025
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

Mostly looks good! Thanks @zhjwpku!

src/iceberg/test/bucket_util_test.cc

// timestamp
// 2017-11-16T22:31:08 in microseconds
std::chrono::system_clock::time_point tp =
Copy link
Member

@wgtmac wgtmac Oct 17, 2025

Choose a reason for hiding this comment

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

No problem!

@zhjwpku zhjwpku force-pushed the implement_literal_ser_de branch from fbc9d10 to 1ea7efd Compare October 17, 2025 16:33
@zhjwpku zhjwpku requested a review from wgtmac October 18, 2025 01:31
@wgtmac wgtmac changed the title feat: Literal support decimal & some refactor feat: support decimal literal and refactor transform utilities Oct 19, 2025
@wgtmac wgtmac merged commit 6e5976c into apache:main Oct 19, 2025
10 checks passed
@zhjwpku zhjwpku deleted the implement_literal_ser_de branch October 21, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmac wgtmac wgtmac approved these changes

@HuaHuaY HuaHuaY Awaiting requested review from HuaHuaY

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Create a dedicated utility file for each Transform category

4 participants

@zhjwpku @HeartLinked @wgtmac @HuaHuaY