andygrove commented on PR #5483:
URL:
https://github.com/apache/datafusion-comet/pull/5483#issuecomment-5441265951
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
The write-up on this one is really good. The explanation of why a stale link
makes AQE lose the stage during replanning is the clearest description of that
failure mode I have seen, and the unit test in `CometExecRuleSuite` that walks
two replanning rounds across all three original-tag shapes is exactly the right
way to pin the behavior down.
Some things I would like to see addressed.
**Only one of the three link-repair blocks is guarded**
`CometExecRule.scala` has three near-identical repair blocks in that same
`transform`, one for `CometExec`, one for `CometShuffleExchangeExec`, and one
for `CometBroadcastExchangeExec`. The new guard is only on the first one. The
rationale in the description talks about a later exchange inheriting the stale
link, so it is not obvious to me why the exchange cases are safe to leave
alone. Can an exchange ever carry a direct `LogicalQueryStage` tag that we
would then clobber? If the answer is no, a sentence in the comment saying so
would save the next reader the same question.
Related to that, those three blocks are copies of each other and now they
have diverged. Would you be willing to pull the repair into a single private
helper that all three cases call, with the guard living inside it? Right now a
future change to the repair logic has to be made in three places and it is easy
to miss one, which is roughly what happened here.
**Metric assertions in the `CometExecSuite` test**
The new test asserts `aggregate.longMetric("elapsed_compute").value > 0`.
That does not tell us anything about the logical-link fix and it is the kind of
timing assertion that eventually flakes on a loaded CI runner. The `output_rows
== 64` check is useful because it confirms we found the right aggregate. Could
the `elapsed_compute` one just be dropped?
**Query shape and runtime**
The regression builds four `range` scans totaling roughly 10k rows and joins
them. That is fine for correctness, but `CometExecSuite` is already one of the
slower suites. Did you check what this adds to the suite's wall clock? If it is
meaningful, it might be worth shrinking the ranges, since the point is the
number of groups on the build side and not the input row count.
**Tracking the general problem**
Is #5482 the right home for the broader question of whether Comet's link
repair is still needed at all in its current form, or is that a separate issue?
The repair exists because `originalPlan` is the source of truth for the link,
and this PR establishes that it is not always the source of truth. It would be
good to have that written down somewhere other than a code comment.
--
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]
