Fix the endless loop in Join and Merge when both suffixes are the same - #7692
Open
HuzaifaChaudary wants to merge 1 commit into
Open
Fix the endless loop in Join and Merge when both suffixes are the same#7692HuzaifaChaudary wants to merge 1 commit into
HuzaifaChaudary wants to merge 1 commit into
Conversation
SetSuffixForDuplicatedColumnNames renames a column that is in both frames. the existing one gets leftSuffix and the new one gets rightSuffix, and it repeats until the two names differ. when both suffixes are the same string the names stay equal after every round, so nothing ends the loop and both names grow forever. it now throws an ArgumentException naming the two parameters and the column. the check sits inside the helper rather than in Join and Merge, because equal suffixes are fine when the two frames share no column name, and nothing is renamed in that case.
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.
Fixes #6128
JoinandMergerename a column that appears in both frames . the existing one getsleftSuffix, the new one getsrightSuffix, and it repeats until the two names aredifferent . when both suffixes are the same string the names stay equal after every round , so
nothing ever ends the loop and both names keep growing .
the reporter hit it by passing the join column name where the suffix goes . michaelgsharp
already named the cause on the issue in 2022 , this is just the patch for it .
it now throws an
ArgumentExceptionnaming the two parameters and the column .the check is inside
SetSuffixForDuplicatedColumnNamesrather than up front inJoinandMerge, because equal suffixes are fine when the two frames share no column name . nothinggets renamed in that case and it works today , so an up front check would break it . i
measured that before deciding where to put it :
three tests added . the first covers all four
JoinAlgorithmvalues with"_same"/"_same"and with
""/"", then checks"_left"/"_right"still gives the full column set . the secondgoes the other way and pins that equal suffixes are still allowed when nothing collides . the
third is the same thing for
Merge.worth saying how the without-the-fix run looks , since it is not an ordinary failure . the
defect is an endless loop , so the tests do not go red , they never finish . i took the fix out
and the run was still going after 150 seconds with no result , at about 92 percent cpu . with
the fix the same six tests finish in 43 ms .
Microsoft.Data.Analysis.Testsis 483 passing , 0 failures , 5 skipped . the tree without thisruns 477 , so the only difference is the six new ones . build is clean with
EnforceCodeStyleInBuild.i also grepped every
.Join(and.Merge(caller insrcandtest, none of them passequal suffixes , so the new guard cannot fire on existing code .