GGraziadei opened a new pull request, #17893:
URL: https://github.com/apache/iceberg/pull/17893
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.1** — `SparkZOrderFileRewriteRunner` 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
