Skip to content

Core, Spark: Introduce shared MultiColumnTerm to de-duplicate Z-order and Hilbert clustering - #17893

Open
GGraziadei wants to merge 1 commit into
apache:mainfrom
GGraziadei:multi-column-term
Open

Core, Spark: Introduce shared MultiColumnTerm to de-duplicate Z-order and Hilbert clustering#17893
GGraziadei wants to merge 1 commit into
apache:mainfrom
GGraziadei:multi-column-term

Conversation

@GGraziadei

Copy link
Copy Markdown
Member

Closes#17657.

Follow-up to #16827, which deliberately mirrored the Z-order surface to keep that diff reviewable. This PR is the deferred de-duplication, along the lines @RussellSpitzer suggested in review ("we'll probably want a MultiColumnTerm or something like that in the future").

What changed

Core — new org.apache.iceberg.expressions.MultiColumnTerm, an immutable Term over an ordered list of column references. Zorder and Hilbert (previously identical apart from the class name) become two-line subclasses. Public constructors and refs() are unchanged; the parsed SQL grammar is untouched.

Spark 4.1SparkZOrderFileRewriteRunner and SparkHilbertFileRewriteRunner move onto a new abstract SparkCurveFileRewriteRunner, parameterised by the internal value column name, the exact per-curve error messages, and the combine step. The shared pieces — internal column schema/sort order, column validation (validZOrderColNames / validHilbertColNames were byte-for-byte the same logic), sortSchema, sortedDF, and the per-column ordered-bytes conversion — now live in one place. Each runner keeps only what is genuinely curve-specific: Z-order its two options and the interleave combine (~120 lines), Hilbert its fixed per-column bit width and the Hilbert-index combine (~75 lines). Adding a further curve is now a small, local change.

Dispatch — the duplicated zorder/hilbert branches in Spark3Util.toIcebergTerm share one reference-extraction helper, and RewriteDataFilesProcedure collapses its two per-curve term lists into a single List<MultiColumnTerm> (mixing curves is detected by distinct term classes; error messages unchanged).

What did not change

Engine-side behaviour is bit-for-bit identical, which the existing tests enforce: every pre-existing Z-order and Hilbert test passes unmodified (TestSparkFileRewriteRunners, the Z-order/Hilbert cases of TestRewriteDataFilesAction, TestRewriteDataFilesProcedure). The only test change is the new TestMultiColumnTerm in core. All user-facing error messages are preserved exactly. Spark 3.5/4.0 are untouched.

Out of scope

The wider "arbitrary function via Expressions" refactor discussed in #16827 review builds naturally on this abstraction but is split out per the issue, to keep this diff mechanical and easy to verify.

@GGraziadei

Copy link
Copy Markdown
MemberAuthor

Please consider GGraziadei#1 as the natural follow-up once this PR is merged. The idea is to extend the grammar to natively support transformations, improving the quality of Z-order and Hilbert clustering.

E.g.

CALL catalog.system.rewrite_data_files(
table => 'db.sample', strategy => 'sort',
sort_order => 'zorder(truncate(4, c2), days(c3))');

@uros-buros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems like a clean mechanical refactor, so thank you @GGraziadei - let's leave up to maintainters (@RussellSpitzer, @nssalian) to take this further

@gimgit

Copy link
Copy Markdown

The de-duplication looks clean, and parameterising the per-curve error strings keeps the user-facing behavior stable.

One place where the refactor changes the failure mode for future extension: in applySortStrategy, anything that is not a Zorder now falls through to action.hilbert(...). Before, a term type the parser didn't recognize ended up in sortOrderFields and failed loudly; now a future MultiColumnTerm subclass would be silently executed as a Hilbert rewrite. An explicit instanceof Hilbert branch plus a throw for anything else would keep the dispatch closed. The mixed-terms error message ("Cannot mix Zorder and Hilbert") has the same generalization gap — it now fires on any two distinct classes.

Minor: refs() used to return Arrays.asList over the internal array (mutable via set), now an ImmutableList — a strictly better contract, but technically a behavior change on a public method; may be worth a line in the PR description.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core, Spark: Introduce a shared multi-column term to de-duplicate Z-order and Hilbert clustering

3 participants

@GGraziadei@gimgit@uros-b