Skip to content

[Iceberg]Fix identity and truncate transform on DecimalType column#21958

Merged
hantangwangd merged 1 commit intoprestodb:masterfrom
hantangwangd:fix_decimal_type_in_partition
Feb 21, 2024
Merged

[Iceberg]Fix identity and truncate transform on DecimalType column#21958
hantangwangd merged 1 commit intoprestodb:masterfrom
hantangwangd:fix_decimal_type_in_partition

Conversation

@hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Feb 18, 2024

Description

When we create partitions on DecimalType columns(using identity or truncate transform) in Iceberg, we will meet various problems because of the inappropriate handling for decimal data.

This PR fix the problems appears when using decimal as a partition column.

Test Plan

  • Test identity transform on column of ShortDecimalType type
  • Test truncate transform on column of ShortDecimalType type
  • Test identity transform on column of LongDecimalType type
  • Test truncate transform on column of LongDecimalType type

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== RELEASE NOTES ==

Iceberg Changes
* Fix identity and truncate transforms on DecimalType columns

@hantangwangd hantangwangd requested a review from a team as a code owner February 18, 2024 11:24
@hantangwangd hantangwangd changed the title Fix identity and truncate transform on DecimalType column [Iceberg]Fix identity and truncate transform on DecimalType column Feb 19, 2024
@steveburnett
Copy link
Contributor

Instead of NO RELEASE NOTE, perhaps a release note entry similar to the following would be appropriate?

== RELEASE NOTES ==

Iceberg Changes
* Fix identity and truncate transforms on DecimalType columns

@hantangwangd
Copy link
Member Author

@steveburnett Thanks for your reminder. Release note has been added!

Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Thanks for this fix. A couple of questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this preserve the precision?

Copy link
Member Author

Choose a reason for hiding this comment

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

It didn't, and the precision would always be 0 in the result value of BigDecimal. But precision of 0 in BigDecimal is ok as it is not a strictly required parameter as scale. It could be gotten by function precision() if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a preconditions check to ensure we don't return just the unscaled value of a LongDecimalType?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, good suggestion. Fixed!

@tdcmeehan tdcmeehan self-assigned this Feb 20, 2024
@hantangwangd hantangwangd force-pushed the fix_decimal_type_in_partition branch from bfe2493 to c6cf611 Compare February 20, 2024 16:29
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

One last nit!

@hantangwangd hantangwangd force-pushed the fix_decimal_type_in_partition branch from a7fb08e to 290b7ca Compare February 21, 2024 00:29
@hantangwangd
Copy link
Member Author

@tdcmeehan Fixed. Please take a look, thanks!

@hantangwangd hantangwangd force-pushed the fix_decimal_type_in_partition branch 2 times, most recently from 5802e3f to d2be45a Compare February 21, 2024 06:34
@hantangwangd hantangwangd force-pushed the fix_decimal_type_in_partition branch from d2be45a to 100d430 Compare February 21, 2024 10:16
@hantangwangd hantangwangd merged commit 4665527 into prestodb:master Feb 21, 2024
@hantangwangd hantangwangd deleted the fix_decimal_type_in_partition branch February 21, 2024 13:04
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants