LuciferYang opened a new pull request, #12750:
URL: https://github.com/apache/gluten/pull/12750
### What changes were proposed in this pull request?
`TransitionCostModel#costComparator` breaks a cost tie on the transited
plan's node names, and did so by subtracting two `String` hashCodes. The
subtraction overflows: `"RowToVeloxColumnar"` hashes to 2056048280 and
`"CHColumnarToCarrierRow"` to -2037667767, so the true difference is
4093716047, past `Int.MaxValue`, and the int result wraps to -201251249. Across
the node names Gluten's transitions actually produce, 46 of the 110 ordered
pairs invert this way. `FloydWarshallGraph#build` uses the comparator to decide
whether a newly found path replaces the incumbent, so an inverted sign can swap
in an equal-cost path that should have lost. The graph is built once per driver
and cached, so the choice then applies to every query in the session.
This switches the tiebreaker to `Integer.compare` on the same two hash
codes. That corrects the sign without changing which path wins any tie that did
not overflow, so no query plan changes.
The narrower fix is deliberate. Comparing the joined strings with
`String#compareTo` would also remove hash collisions, but it changes the sort
key and therefore the winner of every equal-cost tie, not just the overflowing
ones. One such tie is real: ArrowNative to VanillaRow has two paths costing 15
each, `LoadArrowData` plus `ColumnarToRow` versus
`ArrowColumnarToVeloxColumnar` plus `VeloxColumnarToRow`, and their hash codes
are close enough that the subtraction was already correct (-998799209, no
overflow). Under `compareTo` the second path wins instead, which breaks five
assertions in `VeloxTransitionSuite` and, more importantly, rewrites the plan
for every query where an Arrow-native operator feeds a row operator. There is
no evidence the new path is cheaper at runtime, so that belongs in a separate
change with Velox-side review, not smuggled into an overflow fix.
Two limits stay as they were, since removing either changes tie winners.
Distinct name sequences can share a hash code, and `mkString` joins without a
separator, so `Seq("Load", "ArrowData")` and `Seq("LoadArrow", "Data")`
collapse to one string. A tie then falls back on `mutable.Map` iteration order.
The old comment claimed the tiebreaker was there "To make the output order
stable"; the new comment says what it does and what it does not.
### How was this patch tested?
New `TransitionCostModelSuite` in `gluten-core`. `costComparator does not
overflow on distant node name hash codes` fails before the change and passes
after: it asserts the raw subtraction is negative for that pair while the
comparator orders it positive. The other two tests pin that the base cost still
decides before node names, and that identical sequences stay equal; both pass
before and after, so the honest count is one guardrail plus two boundary
assertions.
`TransitionCost` is private, so the suite builds one through a new
`private[transition]` helper on `TransitionGraph`. The suite's cost model is a
three-line subclass of the production `LongCostModel` rather than a hand-rolled
stub, so its `sum`, `diff`, and comparator keep production semantics.
`mvn -Pspark-3.5 -pl gluten-core test` gives 54 tests passing. Cross-version
`test-compile` passes on spark-3.3, spark-3.4, spark-4.0 with scala-2.13, and
spark-4.1 with scala-2.13.
Worth flagging for review: `VeloxTransitionSuite` is the suite that would
catch a tie flip, it lives in `backends-velox` and needs the native library,
and I could not run it locally. CI covering it is what confirms the "no plan
change" claim.
### Was this patch authored or co-authored using generative AI tooling?
No
Closes #12748
--
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]
