jeffw13 commented on PR #5821: URL: https://github.com/apache/datafusion-comet/pull/5821#issuecomment-5670364221
> Coming back to the reachability point from my earlier comment, since it is the one I most wanted your thoughts on. `CometEmptyRelationExecSuite` is still the only end to end test of the operator and it needs `spark.comet.sparkToColumnar.enabled=false` and `spark.comet.shuffle.convertFromSparkPlan.enabled=false` to reach it. At default settings this aggregate shape produces no empty relation under Comet at all, because `AQEPropagateEmptyRelation.getEstimatedRowCount` only learns a row count from a `QueryStageExec` or a `BaseAggregateExec`, and Comet has already turned the inner aggregate into `CometHashAggregateExec`. The comment on `CometAggregateExpressionSerde.supportsMixedPartialFinal` names the same gap, which is why COUNT is excluded there. > > The shapes that do reach `CometEmptyRelationExec` on a default config are the join elimination ones, inner join and left semi join with an empty right side, and none of those has an end to end test. Would you add one? A test that only passes with two non default configs will not fail if a future change makes the operator unreachable by default, and `CometEmptyRelationExecRuleSuite` cannot catch that either because it constructs `EmptyRelationExec` by hand and calls `CometExecRule` directly. It also seems worth saying in #5819 that the `BaseAggregateExec` inference gap is separate work this PR does not close. > > The `innerChildren` route does bring the eliminated subtree back into `treeString`, thanks for that. One side effect worth checking though. `ExtendedExplainInfo.generateTreeString` also walks `innerChildren`, and the wrapped `EmptyRelationExec` is not a `CometPlan`, so it lands in `CometCoverageStats.sparkOperators`. `spark.comet.explain.format` defaults to `verbose` and `supportsExtendedExplainInfo` is unconditionally true on Spark 4.x, so the Comet summary now reports the node Comet just accelerated as an extra un-accelerated Spark operator, and `CometMetricsListener` feeds the same count into the `comet.operators.spark` counter and the `acceleration.ratio` gauge. Comet's renderer does not call Spark's `generateTreeString`, so all it prints there is a bare `EmptyRelation` line with no subtree under it. Would it make sense to have `ExtendedExplainInfo` skip `innerChildren` for this node, or to go back to a `generateTreeString` override behind a small shim so the two outputs agr ee? > > One process note. `Spark SQL Tests (Spark 4.0)` is behind the `run-spark-4.0-tests` label and was skipped, so the `dev/diffs/4.0.4.diff` change here has never been exercised. The 4.1 equivalent did run and passed. Could you apply the label once before this goes in? Yes, i've updated to a join test that properly tests this code path, thanks for pointing that out. I've also corrected the explain issue. Regarding the label, it seems I cannot add labels due to only having read access on the repo, is this something you or @sunchao could help me with? -- 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]
