Skip to content

[Variant] Add variant to arrow for Date64/Timestamp(Second/Millisecond)/Time32/Time64#8950

Merged
alamb merged 3 commits intoapache:mainfrom
klion26:remaining_arrow_types_from_variant
Dec 11, 2025
Merged

[Variant] Add variant to arrow for Date64/Timestamp(Second/Millisecond)/Time32/Time64#8950
alamb merged 3 commits intoapache:mainfrom
klion26:remaining_arrow_types_from_variant

Conversation

@klion26
Copy link
Copy Markdown
Member

@klion26 klion26 commented Dec 4, 2025 •
edited
Loading

Which issue does this PR close?

What changes are included in this PR?

Add support for variant to arrow primitive types(for the remaining arrow primitive types), and some tests to cover them.

For the behavior that can't be cast safely, I'll continue to track them in #8086 and #8873

Self::make_time will return the native value for the given timestamp type
Date64Type::from_naive_date(v) will return the milliseconds elapsed since UNIX epoch

VariantType Arrow Type Logic
Date Date64 datatypes::Date64Type::from_naive_date(v)
Timestamp[_ntz](Micro/Nano) Timestamp[_ntz](Second) - if (timestamp.nano == 0) Self::make_time(timestamp)
- else None
Timestamp[_ntz](Micro/Nano) Timestamp[_ntz](Millisecond) - if (timestamp.nano % 1_000_000 == 0) Self::make_time(timestamp)
- else None
Time Time32(Second) - if (timestamp.nano == 0) v. num_seconds_from_midnight()
- else None
Time Time32(Millisecond) - if (timestamp.nano % 1_000_000 == 0) v.num_seconds_from_midnight() * 1000 + v.nano / 1_000_000
- else Nnoe
Time Time64(Nano) timestamp.num_seconds * 1_000_000_000 + v.nano

Are these changes tested?

Added some new tests

Are there any user-facing changes?

No

@github-actions github-actions bot added the parquet-variant parquet-variant* crates label Dec 4, 2025
@klion26 klion26 force-pushed the remaining_arrow_types_from_variant branch from 69bb43b to 1af7a22 Compare December 4, 2025 02:50
klion26
klion26 commented Dec 4, 2025
View reviewed changes
Copy link
Copy Markdown
Member Author

@klion26 klion26 left a comment

Choose a reason for hiding this comment

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

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

Comment thread parquet-variant-compute/src/type_conversion.rs
});
impl_primitive_from_variant!(datatypes::Time64MicrosecondType, as_time_utc, |v| {
(v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64
Some((v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64)
Copy link
Copy Markdown
Member Author

@klion26 klion26 Dec 4, 2025

Choose a reason for hiding this comment

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

We don't need to handle the case v.nanosecond() % 1000 != 0 here. Thanks for variant_array::canonicalize_and_verify_data_type, we can assume that the input here is always Time64(TimeUnit::Microsecond)(No Time32 or Time64(TimeUnitNanosecond))

@mhilton mhilton mentioned this pull request Dec 4, 2025
Merged
alamb
alamb approved these changes Dec 10, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @klion26 -- I went though this PR and it looks good to me

Comment thread parquet-variant-compute/src/type_conversion.rs
Some(datatypes::Date64Type::from_naive_date(v))
});
impl_primitive_from_variant!(datatypes::Time32SecondType, as_time_utc, |v| {
// Return None if there are leftover nanoseconds
Copy link
Copy Markdown
Contributor

@alamb alamb Dec 10, 2025

Choose a reason for hiding this comment

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

this makes sense to me -- we return None because we can't cast without losing precision

Copy link
Copy Markdown
Member Author

@klion26 klion26 Dec 11, 2025

Choose a reason for hiding this comment

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

Yes, will do this after #8086 and #8873,

Filed an issue(#8982) to track this.

Comment thread parquet-variant-compute/src/variant_get.rs Outdated

perfectly_shredded_variant_array_fn!(perfectly_shredded_time_variant_array_for_time32, || {
Time64MicrosecondArray::from(vec![
Some(1234), // This can't be cast to Time32
Copy link
Copy Markdown
Contributor

@alamb alamb Dec 10, 2025

Choose a reason for hiding this comment

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

it can cast, but the cast is not lossless

Copy link
Copy Markdown
Member Author

@klion26 klion26 Dec 11, 2025

Choose a reason for hiding this comment

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

Yes, updated the comment to make it more clearly.

@alamb alamb merged commit 365a9ec into apache:main Dec 11, 2025
17 checks passed
@alamb
Copy link
Copy Markdown
Contributor

alamb commented Dec 11, 2025

Thanks @klion26

@klion26 klion26 deleted the remaining_arrow_types_from_variant branch December 15, 2025 02:41
@klion26
Copy link
Copy Markdown
Member Author

klion26 commented Dec 15, 2025

@alamb Thanks for the review and merging!

This was referenced Jan 6, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

parquet-variant parquet-variant* crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Variant] Support more Arrow Datatypes from Variant primitive types

2 participants

@klion26 @alamb