andygrove commented on PR #5331:
URL:
https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5299344889
One more, on the gate in `serializePartitions`:
```scala
val reportable = reportableOrdering(scanExec.ordering, output)
if (reportable.nonEmpty) {
val protoOrders = reportable.map(exprToProto(_, output))
if (protoOrders.forall(_.isDefined)) {
commonBuilder.addAllTableSortOrders(protoOrders.map(_.get).asJava)
}
}
```
The comment above it argues the `forall` always holds, and I agree that's
true today. My worry is what happens if it ever stops being true, because the
guard makes the unsafe direction the silent one. If we merge without telling
Spark, we've wasted some work and nothing else. If we tell Spark and then don't
merge, Spark drops the Sort and we return wrong results with no error.
That's not purely hypothetical, because the gate gets evaluated twice
against two different points in time. `outputOrdering` is a lazy val forced
during planning, this runs later off `serializedPartitionData`, and
`reportableOrdering` reads `COMET_ICEBERG_SORT_MERGE_ENABLED` out of the active
`SQLConf` each time. Two dynamic conf reads on separate threads at separate
stages is the kind of thing that holds until it doesn't.
`serializePartitions` only has the one caller, so could we just pass
`outputOrdering` in from the exec and have a single evaluation? Failing that
I'd rather the mismatch threw than silently produced a proto that disagrees
with what we told Spark.
--
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]
