[Variant] Add Variant::Time primitive and cast logic by klion26 · Pull Request #8114 · apache/arrow-rs

klion26 · 2025-08-12T11:34:20Z

Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.

Closes [Variant]: Implement DataType::Time32/Time64 support for cast_to_variant kernel #8055.

Rationale for this change

Add Variant::Time support and cast_to_variant for Variant::Time

What changes are included in this PR?

Add Variant::Time primitive support
Add primitive_time.metadata and primitive_time.value generated from Iceberg Code into parquet-testing
Add cast_to_variant support for Variant::Time

Are these changes tested?

Added tests for the added feature.

Are there any user-facing changes?

No

klion26 · 2025-08-12T12:10:41Z

@alamb please help review this when you're free, thanks.

I've also filed a pr to parquet-testing here

alamb · 2025-08-12T20:47:18Z

Thanks @klion26 !

Add primitive_time.metadata and primitive_time.value generated from Iceberg Code into parquet-testing

Can you please make a PR to parquet-testing repo directly? We'll need to get that repo updated prior to merging this PR

https://github.com/apache/parquet-testing

alamb

Thank you @klion26 -- I think this looks great.

We just need to sort out the parquet-testing pin and I think this would be good to merge

I did have a question about utc normalization that might be worth looking at

alamb · 2025-08-12T20:48:13Z

this logic looks good to me -- thank you

alamb · 2025-08-12T20:48:33Z

perhaps the println is left over

fixed in the followup pr(will push remote after parquet-testing pr has been merged)

alamb · 2025-08-12T20:49:36Z

I wonder if there is a reason not to compare the output json string directly (why look for contains and starts/ends)? One thing a full compare would do is output the actual value in any failure message

Fixed it(in the follow-up pr, will push remote after parquet-testing pr has been merged).

klion26

@alamb thanks for the review. I've filed pr for parquet-testing(apache/parquet-testing#92), and will update the current pr after the parquet-testing pr has been merged.

klion26 · 2025-08-13T05:21:37Z

Fixed it(in the follow-up pr, will push remote after parquet-testing pr has been merged).

klion26 · 2025-08-13T05:21:44Z

fixed in the followup pr(will push remote after parquet-testing pr has been merged)

klion26 · 2025-08-14T07:07:27Z

Close and reopen to trigger ci

klion26 · 2025-08-14T07:22:10Z

@alamb I've updated the pr, and the pr in parquet-testing has been merged. Please take another look when you're free.

The parquet-testing pr has been added in the current pr(the latest commit id in parquet-testing is the same as the latest commit id in apache/parquet-testing master branch). I can change it if there is something wrong.

The CI tests rust/verify MSRV failed because of error: binary cargo-msrv already exists in destination, it seems is not related to the current pr

alamb

Thank you @klion26 -- this is really solid and well done ❤️

I merged up from main to resolve a conflict and I'll then plan to merge it

alamb · 2025-08-14T12:12:55Z

        Ok(())
    }

+    #[test]


alamb · 2025-08-14T19:38:43Z

The CI tests rust/verify MSRV failed because of error: binary cargo-msrv already exists in destination, it seems is not related to the current pr

Yes, I agree this does not seem relate. I did some digging and I think it is related to the binary being cached already. I pushed a commit to fix this (hopefully): afbcb4b

alamb · 2025-08-14T20:27:39Z

Thank you @klion26 for your patience

klion26 · 2025-08-15T03:08:24Z

@alamb thank you for your review and merging!

github-actions bot added the parquet Changes to the parquet crate label Aug 12, 2025

klion26 force-pushed the variant_time branch 2 times, most recently from 46a82fb to f086463 Compare August 12, 2025 12:00

klion26 mentioned this pull request Aug 12, 2025

[Variant] Support Timestamp to variant for cast_to_variant kernel #8113

Merged

alamb reviewed Aug 12, 2025

View reviewed changes

klion26 commented Aug 13, 2025

View reviewed changes

klion26 mentioned this pull request Aug 13, 2025

[Variant] Add support the remaing primitive type(timestamp_nanos/timestampntz_nanos/uuid) for parquet variant #8126

Closed

github-actions bot added parquet-variant parquet-variant* crates and removed parquet Changes to the parquet crate labels Aug 14, 2025

klion26 added 2 commits August 14, 2025 14:49

[Variant] Add Variant::Time primitive and cast logic

803e115

fix style

16b2cad

klion26 force-pushed the variant_time branch from 4b5709e to 81941fd Compare August 14, 2025 06:49

klion26 requested a review from alamb August 14, 2025 06:52

update test

8a522b6

klion26 force-pushed the variant_time branch from 81941fd to 8a522b6 Compare August 14, 2025 06:57

klion26 closed this Aug 14, 2025

klion26 reopened this Aug 14, 2025

Merge remote-tracking branch 'apache/main' into variant_time

1051ef1

alamb approved these changes Aug 14, 2025

View reviewed changes

Comment thread parquet-variant-json/src/to_json.rs

Ok(())

}

#[test]

Copy link
Copy Markdown

Contributor

alamb Aug 14, 2025

Choose a reason for hiding this comment

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

❤️

alamb added 2 commits August 14, 2025 15:08

Merge remote-tracking branch 'apache/main' into variant_time

dfba596

Only install cargo-msrv if needed

afbcb4b

alamb merged commit 48b723f into apache:main Aug 14, 2025
32 checks passed

klion26 deleted the variant_time branch August 15, 2025 03:08

alamb mentioned this pull request Aug 21, 2025

[Variant]: Implement DataType::Time32/Time64 support for cast_to_variant kernel #8055

Closed

                       Ok(())
                   }
+                  #[test]

Conversation

klion26 commented Aug 12, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

klion26 commented Aug 12, 2025

Uh oh!

alamb commented Aug 12, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

klion26 Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

klion26 Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

klion26 Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klion26 Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

klion26 commented Aug 14, 2025

Uh oh!

klion26 commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 14, 2025

Uh oh!

Uh oh!

alamb commented Aug 14, 2025

Uh oh!

klion26 commented Aug 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

klion26 Aug 13, 2025 •

edited

Loading

klion26 Aug 13, 2025 •

edited

Loading

klion26 Aug 13, 2025 •

edited

Loading

klion26 Aug 13, 2025 •

edited

Loading

klion26 commented Aug 14, 2025 •

edited

Loading