fix: bind a RIGHT join's merge arguments to the declared sides - #1097
Open
TKaltofen wants to merge 2 commits into
Open
fix: bind a RIGHT join's merge arguments to the declared sides#1097TKaltofen wants to merge 2 commits into
TKaltofen wants to merge 2 commits into
Conversation
Behavior change: a RIGHT join between two feature groups that resolve to different compute frameworks now returns different results. Before, it either raised a KeyError naming the declared left index column or, where the index columns happened to line up, returned the rows of a join with the two sides exchanged. It now returns the RIGHT join the link declares. run_link swaps destination and source framework for JoinType.RIGHT so the join executes where the declared right group runs, but left the swap_merge_sides flag False. JoinStep._merge_data then handed the destination's data, the declared right group, to the merge engine as its left argument, while the link indices stay bound to the declared feature groups. Left keys a, b, c against right keys a, b returned three rows where a RIGHT join returns two. The flag now follows which declared side the destination framework actually holds, read off the parents in the graph, rather than the position of a framework in the trekker key. Setting it for every RIGHT join would have been wrong in the other direction: when the key reaching run_link is already reversed, the destination is the declared left group and False is correct. Self links and sides sharing one framework are not decidable this way and keep the previous value.
Deciding which declared side holds the join's destination framework by plain issubclass membership answered "both" in two shapes, which sent the decision to its positional fallback and rebound a RIGHT join to the wrong sides again. A declared right group that subclasses the declared left group matches both sides itself. And the parents walked here are the transitive ancestor closure of the join's children, not the join's two parents, so any ancestor anywhere upstream that subclasses a declared side and runs on the destination framework contaminates the answer. Both reach the merge engine with the declared right group's data as its left argument. Each side now keeps only the nodes closest to it in the MRO, ranked independently, which separates a subclass from the base it derives from and drops a distant sibling in favour of the real parent. The MRO distance helper moves to module scope so both the link matcher and the planner read the same one. Two subclasses of the declared left group at the same distance on different frameworks stay undecidable and keep the previous value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
A
JoinType.RIGHTlink between two feature groups that resolve to different compute frameworks bound its merge arguments to the resolved frameworks instead of the declared sides.run_linkswaps destination and source framework for RIGHT so the join executes where the declared right group runs, but leftswap_merge_sidesFalse.JoinStep._merge_datathen handed the destination's data, the declared right group, to the merge engine as its left argument, while the link's indices stay bound to the declared feature groups. Left keys a, b, c against right keys a, b returned three rows where a RIGHT join returns two. Where the index column names did not line up it raised aKeyErrornaming the declared left index instead.The flag now follows which declared side the destination framework actually holds, read off the parents in the graph, rather than the position of a framework in the trekker key. Setting it for every RIGHT join would be wrong in the other direction: when the key reaching
run_linkis already reversed the destination is the declared left group, and False is correct there.Sides are resolved by nearest inheritance distance rather than plain
issubclassmembership, because membership answers "both sides" in two shapes that then fall through to the old behavior:Keeping only the closest nodes per side separates a subclass from the base it derives from and drops a distant sibling in favour of the real parent. The MRO distance helper moved to module scope so the link matcher and the planner share one.
Self links, and two subclasses of one declared side at equal distance on different frameworks, stay undecidable from the declared sides and keep the previous value.
Behavior change
RIGHT joins across compute frameworks now return the join the link declares. Runs that previously raised will now succeed, and runs that returned rows from a join with the sides exchanged will return different rows.
Tests
New end-to-end regression coverage for a plain RIGHT join across frameworks, for a declared right group that subclasses the declared left one, and for a sibling subclass reaching the same child as a second ancestor, all with asymmetric key sets so a wrong binding changes the row set rather than only raising. Plan-level coverage for the reversed-key and inverted-branch orientations that must not swap. The existing planner characterization suites are updated where they pinned the old results.
toxis green: 8978 passed, 170 skipped, 2 xfailed.