[Variant][Shredding] Support typed_access for timestamp_micro/timestamp_nano by klion26 · Pull Request #8401 · apache/arrow-rs

klion26 · 2025-09-22T02:26:12Z

Which issue does this PR close?

Closes [Variant] [Shredding] Support typed_access for Timestamp(Microsecond, _) and Timestamp(Nanosecond, _) #8331 .

Rationale for this change

Add typed_access for Timestamp(Micro, _) and Timestamp(Nano, -)

What changes are included in this PR?

Extract some data gen logic in tests to simplify the test logic (commit 93090d56717a6804e4862c23a0f85030b9f6406d), but it based on some old code(before Add Arrow Variant Extension Type, remove Array impl for VariantArray and ShreddedVariantFieldArray #8392), rebase the master in the last commit
Add typed_access for Timestamp(Micro, _) and Timestamp(Nano, _)
Add test for typed_access for Timestamp(Micro, _) and Timestamp(Nano, _)

Are these changes tested?

Covered by existing and added tests

Are there any user-facing changes?

No

klion26 · 2025-09-23T06:33:22Z

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

parquet-variant-compute/src/variant_get.rs

scovich

LGTM, other than a panic that needs to be removed.

Your call whether to deal with timestamp precision widening in this PR or as a follow-on effort (maybe it depends on whether there's any controversy about the approach)

scovich · 2025-09-23T13:00:59Z

parquet-variant-compute/src/variant_array.rs

+                    )
+                }
+                // Variant timestamp only support time unit with microsecond or nanosecond precision
+                _ => panic!(


Can we return an error instead of panicking?

Also, AFAIK all the other timestamp precisions can be widened losslessly to microsecond; should we consider doing that instead of blowing up?

yes, let's not panic here

ANother potential approach is to return a not yet implemented error -- maybe by matching the supported types explicitly, so like

DataType::Timestamp(TimeUnit::Microsecond, Some(_)) => { generic_conversion_single_value!( TimestampMicrosecondType, as_primitive, |v| DateTime::from_timestamp_micros(v).unwrap(), typed_value, index ) }

instead of

DataType::Timestamp(timeunit, tz) => { match (timeunit, tz) { (TimeUnit::Microsecond, Some(_)) => { generic_conversion_single_value!( TimestampMicrosecondType, as_primitive, |v| DateTime::from_timestamp_micros(v).unwrap(), typed_value, index ) }

I'm always a fan of less nesting, when possible...

Thanks for pointing this out. Yes, TimeUnit::Second and TimeUnit::Millisecond can convert to TimeUnit::MicroSecond losslessly. Using panic here because this value is located in typed_value of the Variant, and from the spec of Variant, there are only Micro and Nano units for Timestamp(with/without timezone), I assumed that this is an error(this could not happen?), I can add the second and Millisecond units if this could happen.

Using panic here because this value is located in typed_value of the Variant, and from the spec of Variant, there are only Micro and Nano units for Timestamp(with/without timezone), I assumed that this is an error(this could not happen?)

Good point! There are definitely unit tests (parquet/tests/variant_integration.rs) that expect non-variant types in typed_value columns to produce an error (cases 127 and 137, specifically). So we should not widen on the read path, but rather reject.

Widening would seem to make sense on the shredding write path, tho?

See also #8435, which I noticed while trying to add struct support

IIUC, we need to widen to TimeUnit::MicroSecond when writing to a variant, as TimeUnit::MicroSecond is the minimum precision we support

Exactly. We have flexibility on the write path to decide whether/how we widen or convert values that don't exactly fit the variant type system, but readers expect the resulting types to always be valid variant shredding types.

alamb

Thank you @klion26 and @scovich

alamb · 2025-09-23T19:24:49Z

parquet-variant-compute/src/variant_array.rs

+                    )
+                }
+                // Variant timestamp only support time unit with microsecond or nanosecond precision
+                _ => panic!(


yes, let's not panic here

ANother potential approach is to return a not yet implemented error -- maybe by matching the supported types explicitly, so like

DataType::Timestamp(TimeUnit::Microsecond, Some(_)) => { generic_conversion_single_value!( TimestampMicrosecondType, as_primitive, |v| DateTime::from_timestamp_micros(v).unwrap(), typed_value, index ) }

instead of

DataType::Timestamp(timeunit, tz) => { match (timeunit, tz) { (TimeUnit::Microsecond, Some(_)) => { generic_conversion_single_value!( TimestampMicrosecondType, as_primitive, |v| DateTime::from_timestamp_micros(v).unwrap(), typed_value, index ) }

alamb · 2025-09-23T19:25:50Z

parquet/tests/variant_integration.rs

@@ -119,23 +106,10 @@ variant_test_case!(30);
 variant_test_case!(31);
 // https://github.com/apache/arrow-rs/issues/8334
 variant_test_case!(32, "Unsupported typed_value type: Time64(Microsecond)");
-// https://github.com/apache/arrow-rs/issues/8331


klion26 · 2025-09-24T14:32:23Z

~~I'm rebasing the main to resolve the conflicts~~
rebased on main

alamb

Thank you @klion26 and @scovich

alamb · 2025-09-24T18:48:20Z

parquet-variant-compute/src/variant_get.rs

+        ])
+    });
+
+    partially_shredded_variant_array_gen!(


thank you for reducing the boilerplate

alamb · 2025-09-24T18:51:04Z

I merged up from main to resolve a conflict

alamb · 2025-09-25T20:17:22Z

@klion26 -- this PR seems to have accumulated conflicts -- I apologize for that. Can you please resolve the conflicts so I can merge it in?

klion26 · 2025-09-26T01:24:28Z

@alamb I've rebased on main and resolved the conflicts. Please take another look, thanks.

alamb

Thank you @klion26 🙏 🙏

github-actions bot added the parquet-variant parquet-variant* crates label Sep 22, 2025

klion26 force-pushed the 8331_support_typed_access_for_timestamp branch 2 times, most recently from 984fbcb to 0748c0e Compare September 22, 2025 02:58

klion26 marked this pull request as draft September 22, 2025 03:28

github-actions bot added the parquet Changes to the parquet crate label Sep 23, 2025

klion26 force-pushed the 8331_support_typed_access_for_timestamp branch from e003775 to aa9f8a9 Compare September 23, 2025 06:20

klion26 marked this pull request as ready for review September 23, 2025 06:30

klion26 commented Sep 23, 2025

View reviewed changes

parquet-variant-compute/src/variant_get.rs Show resolved Hide resolved

scovich approved these changes Sep 23, 2025

View reviewed changes

alamb reviewed Sep 23, 2025

View reviewed changes

klion26 force-pushed the 8331_support_typed_access_for_timestamp branch from 958e8b0 to 3019a2e Compare September 24, 2025 14:40

alamb approved these changes Sep 24, 2025

View reviewed changes

klion26 added 5 commits September 26, 2025 08:50

[Varaint] Extract test data gen logic to macro

1b4361c

[Variant] Support typed access for timestamp(micro&nano)

4518a73

address comment

ccb0e65

fix style

d91fbb1

fix style after rebase

bb4c3eb

klion26 force-pushed the 8331_support_typed_access_for_timestamp branch from 69919eb to bb4c3eb Compare September 26, 2025 01:04

fix

12b97ee

alamb approved these changes Sep 26, 2025

View reviewed changes

alamb merged commit 56cdfa7 into apache:main Sep 26, 2025
19 checks passed

klion26 deleted the 8331_support_typed_access_for_timestamp branch September 29, 2025 07:30

alamb mentioned this pull request Oct 17, 2025

[Variant] [Shredding] Support typed_access for Timestamp(Microsecond, _) and Timestamp(Nanosecond, _) #8331

Closed

Conversation

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

Uh oh!

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 Sep 23, 2025

Uh oh!

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klion26 Sep 24, 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!

scovich Sep 24, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

alamb left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 24, 2025

Uh oh!

alamb commented Sep 25, 2025

Uh oh!

klion26 commented Sep 26, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

klion26 commented Sep 22, 2025 •

edited

Loading

klion26 Sep 24, 2025 •

edited

Loading

scovich Sep 24, 2025 •

edited

Loading

klion26 commented Sep 24, 2025 •

edited

Loading

alamb left a comment •

edited

Loading