Fix position tracking across ratio, IS NULL, GROUP BY, ALTER and dictionary nodes#283
Conversation
…ionary nodes
Positions are stamped from caller-supplied `pos` parameters or read
after the relevant token was already consumed, producing wrong or
overlapping spans in several nodes:
- parseRatioExpr passed the numerator's pos to the denominator too, so
`SAMPLE 1/2` produced two literals with the same NumPos and
overlapping spans. The denominator now starts at its own token.
- SELECT ... WITH TOTALS set statementEnd to the position of WITH,
captured before consuming it. It now ends at the TOTALS token.
- GroupByClause.GroupByEnd was set to p.Pos() after the clause - the
START of the next token - so the span swallowed trailing whitespace.
It now ends at the last token of the clause (expression, ALL, or the
WITH CUBE/ROLLUP/TOTALS keyword).
- The ALTER TABLE ... DETACH branch consumed the keyword before taking
p.Pos(), unlike all 12 sibling branches, so the clause's Pos pointed
at PARTITION. It now points at DETACH.
- IsNullExpr/IsNotNullExpr stamped IsPos with the position AFTER the
consumed NULL (the next token's start) and their End() stopped at the
operand, excluding "IS [NOT] NULL" entirely. IsPos is now the IS
keyword, a new NullEnd field marks the end of NULL, Pos() starts at
the operand and End() returns NullEnd.
- DictionaryAttribute.End() returned NamePos + len("IS_OBJECT_ID")
and friends - the attribute NAME's start plus a keyword length,
landing mid-token for any realistic name/type. A new AttrEnd field
tracks the real end of the last property.
- AlterTableDropPartition.End() computed a.Settings.End() and discarded
the result (missing return), excluding the SETTINGS clause from the
span.
Golden JSON diffs are exclusively position-field values plus the two
new fields (NullEnd, AttrEnd); no format output changed. Span
assertions live in the new position_test.go.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 154087443d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return nil, err | ||
| } | ||
| if expr != nil { | ||
| groupByEnd = expr.End() |
There was a problem hiding this comment.
When parsing GROUP BY CUBE(...), ROLLUP(...), or GROUPING SETS(...) with no trailing WITH modifier, expr is a ParamExprList, whose End() is the position of the closing ) rather than the byte after it. Assigning GroupByEnd from expr.End() makes both GroupByClause.End() and the enclosing SelectQuery.End() one byte short (for example, SELECT a FROM t GROUP BY CUBE(a) reports 31 instead of len(sql)==32), so these supported GROUP BY forms still exclude their final token from the span.
Useful? React with 👍 / 👎.
1 participant
Summary
A cluster of wrong-span bugs, all instances of the same root pattern: positions stamped from caller-supplied
posparameters, or read from the stream after the relevant token was already consumed.SAMPLE 1/2denominatorNumPos= numerator's pos (overlapping spans)SELECT ... WITH TOTALSstatementEnd= position ofWITHTOTALSGroupByClause.GroupByEndALL, orWITH CUBE/ROLLUP/TOTALS)ALTER TABLE t DETACH ...clause PosPARTITION(keyword consumed beforep.Pos(), unlike all 12 sibling branches)DETACHa IS [NOT] NULLIsPos= next token afterNULL;End()= operand end (span excludedIS NULLentirely)Pos()= operand start,IsPos= theISkeyword,End()= newNullEndDictionaryAttribute.End()NamePos + len(\"IS_OBJECT_ID\")— name start plus a keyword length, landing mid-tokenAttrEndtracks the real endAlterTableDropPartition.End()a.Settings.End()and discarded the result (missingreturn)Verification
While auditing I also re-checked two suspected sites that turned out to be correct (no change):
parseProjectionSelect'sRightParenPosandSYSTEM ... REPLICATED SENDS'StatementEndboth capture the token before consuming it.AST impact
New fields:
IsNullExpr.NullEnd,IsNotNullExpr.NullEnd,DictionaryAttribute.AttrEnd;IsNullExpr.Pos()now starts at the operand (consistent withIsNotNullExpr). Golden JSON diffs are exclusively position-field values plus these fields; no format output changed. Exact span assertions live in the newposition_test.go.🤖 Generated with Claude Code