LukaZdravic commented on PR #58776:
URL: https://github.com/apache/spark/pull/58776#issuecomment-5680422990
> * `SQLMetricsSuite`: the new `SortMergeAsOfJoin metrics` test locates the
operator with
> `executedPlan.collectFirst { case _: SortMergeAsOfJoinExec }`. That
plain `TreeNode`
> lookup is safe here only because the suite mixes in
`DisableAdaptiveExecutionSuite`, so
> the plan is not wrapped in `AdaptiveSparkPlanExec` — consistent with the
sibling join
> metric tests. The three `numOutputRows` cases pin the inner-drop,
left-outer null-pad,
> and NULL-equi-key null-pad increment sites. Good coverage.
> * `SortMergeAsOfJoinSuite`: `checkAnswerAndSpill` correctly uses
> `AdaptiveSparkPlanHelper.collectFirst` (this suite does not disable AQE,
so the AQE
> unwrapping matters). Nice helper.
>
> On the earlier `bufferRightGroup` coverage point: it now looks covered by
the equi-key cases in the forward/nearest spill tests, which use multi-row
groups to actually exceed the threshold. The backward-join equi-key case still
uses plain `checkAnswer`, which is reasonable since its data has at most one
right row per group and would not spill — a one-line comment noting that
asymmetry would help future readers, but it is optional.
>
> Nothing blocking.
Thanks! Added the one-line comment on the backward-join equi-key case. It
has at most one right row per group, so it does not spill, which is why it
keeps plain checkAnswer while the forward and nearest cases use
checkAnswerAndSpill.
--
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]
