Skip to content

[Variant]: Implement DataType::Decimal32/Decimal64/Decimal128/Decimal256 support for cast_to_variant kernel#8101

Merged
alamb merged 3 commits intoapache:mainfrom
liamzwbao:issue-8059-decimal-to-variant
Aug 13, 2025
Merged

[Variant]: Implement DataType::Decimal32/Decimal64/Decimal128/Decimal256 support for cast_to_variant kernel#8101
alamb merged 3 commits intoapache:mainfrom
liamzwbao:issue-8059-decimal-to-variant

Conversation

@liamzwbao
Copy link
Copy Markdown
Contributor

@liamzwbao liamzwbao commented Aug 9, 2025

Which issue does this PR close?

Rationale for this change

See the linked issue.

What changes are included in this PR?

Created a new macro to convert Arrow decimal to variant decimal. Support Decimal32/Decimal64/Decimal128/Decimal256 for cast_to_variant.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, new variant casting supported

@liamzwbao liamzwbao marked this pull request as ready for review August 9, 2025 17:00
@liamzwbao liamzwbao force-pushed the issue-8059-decimal-to-variant branch from c1feb1b to ffb3509 Compare August 12, 2025 03:35
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.

Thank you @liamzwbao -- this makes sense to me

Decimal256Type,
as_primitive,
|v: i256| {
// Since `i128::MAX` is larger than the max value of `VariantDecimal16`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since DataType::Decimal256 can store values larger than can be stored in VariantDecimal16, I don't think this comment is accurate -- don't we need another check for overflow?

But I see that the tests covert the case of i256::MAX so this seems reasonable to me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Improved the comments to make it clearer. What I was trying to say here is that if an i256 cannot fit into i128, then it can not fit into VariantDecimal16 either.

So here we can first convert i256 to i128 and process it like i128.

Arc::new(
Decimal128Array::from(vec![
Some(i128::MIN),
Some(-max_unscaled_value!(128, DECIMAL128_MAX_PRECISION)),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be DECIMAL256_MAX_PRECISION?

Suggested change
Some(-max_unscaled_value!(128, DECIMAL128_MAX_PRECISION)),
Some(-max_unscaled_value!(128, DECIMAL256_MAX_PRECISION)),

Same question for the other uses below too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess you meant the Decimal256 tests. The reason I didn't use DECIMAL256_MAX_PRECISION is that DECIMAL128_MAX_PRECISION can already cause the overflow in the test cases.

I have updated the test cases to include the min/max value that can be cast from Decimal to VariantDecimal so these should be clearer

@github-actions github-actions Bot added the parquet-variant parquet-variant* crates label Aug 13, 2025
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.

Thank you @liamzwbao

@alamb alamb merged commit 536524b into apache:main Aug 13, 2025
12 checks passed
@liamzwbao liamzwbao deleted the issue-8059-decimal-to-variant branch August 13, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parquet-variant parquet-variant* crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Variant]: Implement DataType::Decimal32/Decimal64/Decimal128/Decimal256 support for cast_to_variant kernel

2 participants