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: [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]
