andygrove commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5496797885
A few more from another pass over the current head: On the `require(protoOrders.forall(_.isDefined), ...)` in `serializePartitions` — thanks for closing the silent-drop hole, but the failure mode is now an `IllegalArgumentException` at every executor task rather than a planning-time fallback to Spark. If the `output` binding ever drifts between `CometScanRule.reportableOrdering` (where the gate ran and stashed `metadata.reportedOrdering`) and this call — an aliasing pass, an exprId rewrite, something along those lines — a query that would previously have executed on Spark hard-fails at exec startup. Could this raise an unchecked exception higher up so the whole scan falls back cleanly, rather than `require` at task start? Also, `COMET_ICEBERG_SORT_MERGE_MAX_FILES_PER_PARTITION` has no `checkValue` bound, and the proto field is `uint32`. A user passing `-1` (or any negative int) casts to `4294967295` on the wire and the cap is effectively gone — a partition with tens of thousands of files opens all of them concurrently, which is exactly what the cap was added to prevent. The sibling `COMET_ICEBERG_DATA_FILE_CONCURRENCY_LIMIT` has `.checkValue(v => v > 0, ...)`; want to add the same here? Small one: the doc string for `COMET_ICEBERG_SORT_MERGE_ENABLED` reads "When disabled, the scan native and the ordering is still reported and honoured" — missing a verb after "scan". That description gets surfaced in config listings and generated docs. -- 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]
