andygrove commented on PR #5514:
URL:
https://github.com/apache/datafusion-comet/pull/5514#issuecomment-5608172692
Both remaining P2s are addressed in ac8e5a1c8, and the lint failure at
`CometPlanOnlySuite.scala:272` is gone (an `s` prefix on a string with no
substitution).
### Use the captured settings throughout conversion
You are right that the three-flag snapshot only covered the gate, and I went
looking for a way to make it govern the whole preview before concluding it
cannot be done. `SparkPlan.conf` is:
```scala
override def conf: SQLConf = {
if (session != null) { session.sessionState.conf } else { super.conf }
}
```
with `@transient final val session = SparkSession.getActiveSession.orNull`
captured at construction. So `op.conf` is the live conf of whichever session
each node was built under. `SQLConf.withExistingConf` cannot redirect it,
because it never consults the thread-local. A cloned session with the snapshot
as its conf cannot either, because the nodes already exist and
`withNewChildren` returns `this` when the children are unchanged, so most of
the tree is never reconstructed. And `CometExecRule` reads `op.conf` in six
places, including the per-operator gate at
`handler.enabledConfig.forall(_.get(op.conf))`, the strict-fallback check and
the shuffle checks. Injecting the snapshot would have covered `isCometLoaded`
and the two rules and missed all of those, which is the partial fix you were
objecting to, only harder to see.
So the snapshot is now used as a check rather than an override. It records
every SQL conf that was set, not just Comet's, and at report time anything that
differs from the live session means the preview would describe a configuration
the query never ran under. The report is skipped and the diagnostic names the
keys that moved. Better nothing than coverage numbers for a configuration that
never executed.
The gate is deliberately excluded from that comparison, so the case the
snapshot was added for still works: restoring
`spark.comet.explain.planOnly.enabled` before the callback runs does not drop
the report. That flag decides only whether to report, and the snapshot remains
the authority for it.
The gated test is now two tests sharing one helper, so the comparison you
asked for is against a control rather than against nothing:
```
- a report survives the setting being restored before the callback runs
- a report is skipped when a conversion setting changed before the callback
runs
```
The first still asserts one report and now also asserts it carries `Comet
accelerated` numbers. The second flips `spark.comet.exec.sort.enabled` instead
of the gate, and asserts one line that says `settings changed since it was
planned`, that it names the changed key, and that it carries no coverage
numbers.
### Keep exchange identity restoration independent of projection support
Fixed by not adding a node at all. The subtree's own attribute IDs are
rewritten to the wrapper's, everywhere they appear beneath it, instead of being
re-labelled by a `ProjectExec` of aliases. That reads no user setting and adds
no eligible operator, so it also answers your abstraction point about identity
bookkeeping moving the number the report exists to state.
The reuse test is now parameterized on `spark.comet.exec.project.enabled`.
Your prediction was exact; with the projection approach still in place and only
that flag flipped:
```
- a reused exchange keeps its output IDs so consumers still convert
(project.enabled=true)
- a reused exchange keeps its output IDs so consumers still convert
(project.enabled=false) *** FAILED ***
Comet accelerated 10 out of 14 eligible operators (71%) ... did not
contain "CometHashJoin"
```
Both pass with the rewrite. All 24 `CometPlanOnlySuite` tests pass on the
default profile, spotless and scalastyle are clean.
Two things about the rewrite worth stating, since it is more
invasive-looking than an added node. It walks the whole subtree rather than
just the root because almost every operator derives `output` from its children,
so the producing leaf has to be rewritten for the root to present the new IDs;
that is safe because attribute IDs are globally unique, so an ID in the root's
output means the same attribute wherever it appears below, and intermediate
attributes not in the root's output are untouched. And the direction is
load-bearing: rewriting the consumers to reference the shared subtree's IDs
instead would merge the two copies of a self-join onto one set of attributes,
which is precisely what `ReusedExchangeExec`'s re-aliasing exists to prevent.
--
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]
