parthchandra commented on PR #5331:
URL:
https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5518378219
Thanks Andy. Addressed all three -
>
> 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?
good point — moved the binding to planning time. CometScanRule now binds the
reported ordering to proto against the scan's output up front
(serializeReportedOrdering), and if that fails it's just another reason
orderingHonored is false, so the scan falls back to Spark during planning
instead of hard-failing at exec startup. The serde now writes the already-bound
protos, so the require is gone.
>
> 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?
added .checkValue(v => v >= 0, ...), matching the sibling. 0 stays
meaningful ("never merge, honor via sort"); negatives are now rejected instead
of wrapping to 4294967295 and disabling the cap.
>
> 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.
fixed — "the scan stays native"
--
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]
