Repository navigation
Fix CUBE and GROUPING SETS merging groups from different sets to close #644 - #2577
Open
joshuablac wants to merge 1 commit into
Open
joshuablac wants to merge 1 commit into
joshuablac wants to merge 1 commit into
Conversation
…AlaSQL#644 Groups were keyed only by the values of their grouping columns, so with several grouping sets a group from one set could collide with a group from another set that had the same values, e.g. (a=2) from CUBE(a, b) and (b=2). The rows were then summed into one group and the other group was missing. The group key now includes the grouping set index when the query has more than one grouping set. Plain GROUP BY keys are unchanged. test015 asserted the merged totals; it now expects the correct eight groups. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Closes #644
Problem
With
CUBEorGROUPING SETS, a group from one grouping set can be merged with a group from another set when both have the same values. That double-counts one group and drops the other:develop(null, null)(2, null)(null, 2)(2, 2)(null, 4)(2, 4)GROUP BY GROUPING SETS(a, b)on the same data gives[(2,null,50), (null,4,20)]ondevelop. The correct result is[(2,null,35), (null,2,15), (null,4,20)].Cause
compileGroupkeys each group inxgroupsby its grouping column values only (a + "`" + b). The(a)group fora = 2and the(b)group forb = 2therefore both get the key"2", and the second set's rows are added to the first set's group.Fix
When the query has more than one grouping set, the key is prefixed with the grouping set's index. A query with a single grouping set (plain
GROUP BY) keeps exactly the same keys and code as before.xgroupsis only used inside the compiled group function.Tests
test/test644.js: the issue'sCUBE,GROUPING SETS(a, b), aCUBEwithCOUNT(*)over several rows, and a check thatROLLUPand plainGROUP BYare unchanged.test/test015.js. It asserted the merged totals forGROUPING SETS(a,b), c. For example, it expected(a=0, c=0)to be1000, which is the(a=0,c=0)group (400) plus the(b=0,c=0)group (600). It now expects the eight correct groups and checks all of them, not just the first four.developand pass with this change.bun test, same command asyarn test-only): 2253 pass, 213 skip. The only failures are the 2 CLI tests intest2149.js, which fail the same way on unmodifieddevelopin my local setup (a space in my checkout path, and the version-injection step ofbuild.shnot running).This fix was written with AI assistance (Claude Code) and verified locally as described above.
🤖 Generated with Claude Code