nssalian commented on PR #16827:
URL: https://github.com/apache/iceberg/pull/16827#issuecomment-5122662316
Hey @GGraziadei thank you for this. I took some time to look at this and
here are some high level suggestions:
1. Adding a new public sort strategy to the RewriteDataFiles API is a design
decision, not just an implementation, so it'll want community buy-in before a
deep review. I know you sent a note to the list, but if you have a document
that would help ground a lot of what you are proposing i.e. the motivation, the
benefit, etc. There's longstanding prior interest here (see #3836). Worth
getting something concretely written and linking it here if you haven't already.
2. Docs: docs/docs/spark-procedures.md documents zorder(...) in the
sort_order row and has an options section for the zorder sort strategy, but
there's no hilbert equivalent yet. This would need to be added.
3. Spark version scope: This only touches spark/v4.1, but v4.0 is also
supported. Developing in the latest version first is fine, but could you state
the intent explicitly in the PR description (e.g. "4.1 first, backport to 4.0
to follow") so the scope reads as intentional?
4. Hilbert.java is currently identical to Zorder.java. Mirroring is the
right call for now; a shared base class is worth considering, but I'd leave
that as a follow-up rather than block this PR on it.
5. Benchmarks. I appreciate how candid the benchmark writeup is. The
file-pruning result (16 → 11 files scanned, deterministic planning-time
measurement) is the one I'd lead with. The latency numbers are confounded by
the swap-thrashing you noted (Z-order CoV 20-52% vs Hilbert 3-11%), so I'd
either drop them or reproduce on a box that isn't swapping, and capture scanned
bytes as well as file counts. A rerun at a fixed file count would make the
pruning claim airtight.
One more measurement that would be compelling if it's feasible: whether the
tighter clustering reduces write amplification on subsequent upserts - i.e., a
MERGE/copy-on-write touching fewer data files because updated
rows are more localized than under z-order. It's data-dependent (only when
the merge keys align with the clustering columns), but it would speak directly
to a real-world benefit beyond read pruning.
6. SparkHilbertUDF serialization. The transient ThreadLocal + readObject
re-init pattern matches SparkZOrderUDF, so this looks right. Since the UDF
ships to executors, can you just confirm testHilbertSort actually exercises a
multi-partition shuffle so we know the reused buffer round-trips after
deserialization?
--
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]
