Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Iceberg] Fix argument order in bucket transforms #24309

Conversation

ZacBlanco
Copy link
Contributor

@ZacBlanco ZacBlanco commented Dec 31, 2024

Description

Swap arguments to their correct positions in the bucketValues function.

Motivation and Context

Argument ordering is wrong and can cause incorrect data returned in queries on bucketed tables. This does not affect writing of the tables tables

Impact

  • No impact to existing users. Queries which triggered improper behavior before should now return data correctly. The codepath for this only occurred in the IcebergPlanOptimizer. It's unlikely that any users encountered this as it seems the codepath required to hit this error is gated by a conditional which prevents us from utilizing the "valueTransform" with bucketed partitioning functions. code.

Test Plan

  • Existing tests

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Dec 31, 2024
@prestodb-ci prestodb-ci requested review from a team, pdabre12 and psnv03 and removed request for a team December 31, 2024 17:10
@ZacBlanco ZacBlanco force-pushed the upstream-iceberg-fix-bucket-transform-args branch from 1c2f739 to 455c3bf Compare December 31, 2024 17:12
Position and count seemed to be swapped accidentally in some cases
@ZacBlanco ZacBlanco force-pushed the upstream-iceberg-fix-bucket-transform-args branch from 455c3bf to bfcca31 Compare December 31, 2024 17:13
@ZacBlanco ZacBlanco marked this pull request as ready for review January 1, 2025 02:25
@ZacBlanco ZacBlanco requested a review from a team as a code owner January 1, 2025 02:25
@ZacBlanco ZacBlanco requested a review from presto-oss January 1, 2025 02:25
@ZacBlanco
Copy link
Contributor Author

ZacBlanco commented Jan 1, 2025

@hantangwangd FYI after further analysis I don't believe it is actually possible to trigger this codepath because the error occurs only for the valueTransform function. The codepath which actually calls the valueTransform function prevents the bucket transform from being called here

So we won't be able to write a test for this. It is more like a cosmetic change.

@ZacBlanco ZacBlanco merged commit 51806b4 into prestodb:master Jan 1, 2025
58 checks passed
@hantangwangd
Copy link
Member

@ZacBlanco Thanks for the analysis. You are right that the constructed bucket ValueTransform is not called anywhere since enforced pushdown optimization is not applicable for bucket transform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants