Re: [PR] Core, Spark: Introduce shared MultiColumnTerm to de-duplicate Z-order and Hilbert clustering [iceberg]

["gimgit (via GitHub)" <gi...@apache.org>]

gimgit commented on PR #17893:
URL: https://github.com/apache/iceberg/pull/17893#issuecomment-5524782209

   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.
   


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org