GGraziadei commented on PR #16827: URL: https://github.com/apache/iceberg/pull/16827#issuecomment-5151873435
Hi @nssalian , Thank you again for your message and for taking the time to review this PR. Starting with your first point, thank you for bringing it up. I have prepared a design document and shared it here https://docs.google.com/document/d/1zIvnqcjG1qnHvNYnOacx3baykE4JPlJbmwe6u7ALlHc/edit?usp=sharing . I've also linked it from the PR description to provide more context around the motivation and expected benefits of introducing Hilbert as a new public sort strategy. For now, I have granted comment access to the document. If anyone would like to contribute directly, please let me know your email address and I'll be happy to grant editing permissions. I hope this helps gather feedback and encourages further discussion from the community. Regarding the documentation, thanks for pointing that out. I have updated the PR description to explicitly state "4.1 first, backport to 4.0 to follow." I also added the reference, as suggested by Russell, to the Hilbert algorithm implementation used as the basis for this PR. In addition, I updated ` docs/docs/spark-procedures.md ` to include the `hilbert(c1,c2,....,cn)` sort strategy. If the API grammar changes during the design discussion, I'll update the documentation accordingly. Now, moving on to the technical points. 6. SparkHilbertUDF serialization Confirmed `testHilbertSort` exercises a genuine multi-partition shuffle on executors, so `readObject` is executed. The test forces `TARGET_FILE_SIZE_BYTES ` to half the average file size and asserts that more than 40 output files are written from a single file group https://github.com/apache/iceberg/pull/16827/changes#diff-3f078bcb19378f3b0e2fd46061c5ddecf34f751da6d6ef97ed9f78b9d65817fbR1873-R1882. `SparkShufflingFileRewriteRunner` derives the shuffle width directly from this (numShufflePartitions = Math.max(1, expectedOutputFiles * numShufflePartitionsPerFile)), resulting in approximately 40 shuffle partitions rather than a single partition. 5. Benchmark I completely agree that I can rerun the benchmarks under more controlled conditions, and I'm happy to do so. That said, I believe the overall trend is already quite clear. Beyond the experimental results, there is also strong theoretical support in the literature demonstrating that Hilbert curves provide better locality preservation than Z-order. The mathematical properties of Hilbert space-filling curves have been studied extensively, and they explain why the observed improvements are expected rather than accidental. There is also practical evidence from the data lake ecosystem: both Delta and Apache Hudi already provide Hilbert-based clustering implementations. While every implementation has its own design choices, I think their adoption provides additional confidence that this is a meaningful optimization to explore in Iceberg as well. Regarding the suggestion to report the exact number of scanned bytes, I mentioned my reasoning previously on the dev@ thread, but I'll summarize it here as well. In my opinion, scanned bytes are highly dependent on the storage layout and the specific Hadoop implementation. I would prefer not to introduce additional variables that could add noise to the evaluation. In these benchmarks every data file is approx 16 MB and contains a single parquet row group. Because of the way Spark reads files, once a row group is selected it is read in its entirety. As a consequence, the number of scanned bytes is effectively proportional to the number of scanned files, making it straightforward to derive one metric from the other by multiplying by the file size. If we want to investigate the impact of Parquet configuration parameters such as row-group sizing, page indexes, or different row-group counts per file, I think that deserves a separate discussion or follow-up issue. Those are interesting optimization topics on their own, but for this proposal I'd prefer to keep the scope focused on evaluating the geometric clustering strategy itself. Your suggestion about measuring the impact on `MERGE`/copy-on-write operations is an excellent one. You're absolutely right that this is currently missing from the benchmark suite, and I plan to add it. To keep the scope manageable while still producing a meaningful comparison, I'll initially focus on metrics such as the number of ` deleted data files` during copy-on-write operations. I think this is a good proxy for write amplification and should provide a practical way to evaluate whether improved clustering translates into less data rewriting during updates. If the results are encouraging, we can always extend the benchmark suite with additional write-path metrics in a follow-up. Thanks again for the thoughtful review and for all the suggestions. -- 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]
