fix(having): rewrite aggregates nested in BETWEEN / IN / CASE (P9)#37
Merged
Conversation
HavingAliasTransformer's two traversals matched only FunctionCall, BinaryOp and Not before falling into a catch-all, so an aggregate reached through any other operator was never collected, never promoted into SELECT, and never rewritten to its alias. The predicate then silently did not filter -- wrong rows in both directions, no error: HAVING COUNT(*) BETWEEN 1 AND 2 1 row -> 4 rows (under-filters) HAVING COUNT(*) IN (4, 5) 2 rows -> 0 rows (over-filters) HAVING CASE WHEN COUNT(*) > 2 ... 2 rows -> 0 rows Migrate both onto the walk helpers (R2). Each now reads "handle the aggregate, delegate the rest": 74 lines of hand-rolled match become 24, and the catch-all that caused this is gone. The aggregate arm deliberately returns early rather than delegating -- that early return is what preserves the "no nested aggregates" invariant the old code kept by hand. A test pins it. Behaviour check, since map_children descends into WindowSpec::order_by and the old walker did not: the subquery boundary now works in our favour. map_children does not enter a nested SelectStatement, so an aggregate belonging to a subquery's own scope is correctly left for that scope rather than being claimed by the outer HAVING. Corpus: having_between, having_in_list and having_case drop their expect = "DIFFER" and are plain AGREE cases. having_not stays a GAP, exactly as P10 predicted -- evidence the two entries were correctly split. Full gate holds at 83 cases. Closes P9. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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 P9. First of the non-crossing transformers in the R2 migration, and the one the doc flagged as a fix, not a refactor.
The bug
HavingAliasTransformer's two traversals matched onlyFunctionCall,BinaryOpandNotbefore a_ => {}catch-all. An aggregate reached through any other operator was never collected, never promoted into SELECT, and never rewritten to its alias — so the predicate silently didn't filter. Wrong rows in both directions, no error:HAVING COUNT(*) BETWEEN 1 AND 2HAVING COUNT(*) IN (4, 5)HAVING CASE WHEN COUNT(*) > 2 THEN 1 ELSE 0 END = 1HAVING COUNT(*) > 2always worked, which is why it went unnoticed.The fix
Both functions move onto the
walkhelpers and now read handle the aggregate, delegate the rest. 74 lines of hand-rolled match become 24, and the catch-all responsible is gone.The aggregate arm returns early rather than delegating — that early return is precisely what preserves the "no nested aggregates" invariant the old code kept by hand. There's a test pinning it, since it would be an easy thing to "tidy up" into a bug later.
Behaviour check
The doc requires a per-transformer check rather than a blanket "pure refactor" claim, because
map_childrendescends intoWindowSpec::order_byand these walkers didn't. Result: no regression, and one genuine improvement — the subquery boundary now works in our favour.map_childrendoesn't enter a nestedSelectStatement, so an aggregate belonging to a subquery's own scope is left for that scope instead of being claimed by the outer HAVING. The opaque default is load-bearing here, not incidental.Verification
having_between,having_in_list,having_casego DIFFER → AGREE; theirexpectis dropped. Full gate holds at 83 cases (72 AGREE / 1 DIFFER / 10 GAP).having_notstays a GAP, exactly as P10 predicted it would — good evidence the two entries were correctly split rather than being one finding.cargo testtoo.🤖 Generated with Claude Code