Skip to content

Commit 0914ece

Browse files
ajitpratap0Ajit Pratap Singhclaude
authored
feat: implement NULLS FIRST/LAST support for ORDER BY clauses (SQL-99 F851) (#115)
* feat: implement NULLS FIRST/LAST support for ORDER BY clauses (SQL-99 F851) This commit implements SQL-99 Feature F851 for deterministic NULL ordering in ORDER BY clauses, enhancing SQL-99 compliance to ~80-85%. Changes: - Created OrderByExpression struct wrapping Expression with ordering metadata - Ascending bool for ASC/DESC direction - NullsFirst *bool for tri-state NULLS handling (nil=default, true=FIRST, false=LAST) - Updated parser to handle NULLS FIRST/LAST in SELECT ORDER BY and window functions - Modified AST traversal in extract.go to unwrap OrderByExpression - Enhanced SQL formatter to output NULLS FIRST/LAST clauses - Updated object pooling for OrderByExpression cleanup - Fixed tests to use new OrderByExpression type Impact: - SELECT statements now support: ORDER BY col DESC NULLS FIRST - Window functions now support: OVER (PARTITION BY x ORDER BY y NULLS LAST) - Multi-column ordering with mixed NULL handling supported - Maintains backward compatibility (NULLS handling is optional) Technical details: - OrderBy field changed from []Expression to []OrderByExpression in: - ast.SelectStatement - ast.WindowSpec - Pointer semantics for NullsFirst allows database-default behavior when nil - All dependent code updated: extraction, formatting, tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: reduce sustained load test duration when race detection is enabled The TestSustainedLoad_VaryingWorkers test was timing out after 60 seconds when race detection was enabled in CI. Race detection adds significant overhead (~10-20x slowdown), causing the original 5s duration × 5 worker configurations to exceed the 60s test timeout. Changes: - Reduce test duration from 5s to 2s when race detection is enabled - Reduce worker counts from [10, 50, 100, 200, 500] to [10, 50, 100] - Utilize existing raceEnabled constant from performance_regression_*.go files This ensures tests complete within the 60s timeout while still validating concurrent behavior. Test now completes in <8s with race detection. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * test: add comprehensive NULLS FIRST/LAST test coverage Add 15+ test cases covering NULLS FIRST/LAST functionality in: - SELECT statement ORDER BY clauses (7 tests) - Window function ORDER BY clauses (7 tests) - Real-world SQL queries (1 test) Test coverage includes: - Basic NULLS FIRST and NULLS LAST syntax - Combination with ASC/DESC ordering - Multiple columns with mixed NULLS behavior - Default behavior (no NULLS clause) - Function calls in ORDER BY expressions - Window functions with PARTITION BY and ORDER BY - Complex window specifications with frame clauses Updated convertTokensForWindowFunctions() to handle NULLS, FIRST, and LAST keywords for proper token conversion. Note: Tests for advanced features like column aliases (AS keyword) and complex expressions (arithmetic in ORDER BY) are documented but disabled as those features aren't yet supported by the parser. Addresses review comment from PR #115 requiring dedicated test coverage for NULLS FIRST/LAST implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: extract duplicate NULLS parsing logic to helper method Extract duplicate NULLS FIRST/LAST parsing code from window function and SELECT ORDER BY parsing into a shared helper method `parseNullsClause()`. Changes: - Add parseNullsClause() helper method in parser.go:734-752 - Replace duplicate parsing logic at lines 601-615 (window functions) - Replace duplicate parsing logic at lines 1108-1122 (SELECT ORDER BY) - Helper returns (*bool, error) for tri-state null ordering This refactoring: - Eliminates ~26 lines of duplicate code - Improves maintainability with single source of truth - Maintains identical parsing behavior (all tests pass) - Addresses PR #115 review comment on code duplication All 153 parser tests pass, including 15 NULLS FIRST/LAST tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Ajit Pratap Singh <ajitpratapsingh@Ajits-Mac-mini.local> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 08c36c6 commit 0914ece

11 files changed

Lines changed: 573 additions & 37 deletions

File tree

‎cmd/gosqlx/cmd/sql_formatter.go‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,28 @@ func (f *SQLFormatter) formatSelect(stmt *ast.SelectStatement) error {
162162
f.writeNewline()
163163
f.writeKeyword("ORDER BY")
164164
f.builder.WriteString(" ")
165-
f.formatExpressionList(stmt.OrderBy, ", ")
165+
for i, orderBy := range stmt.OrderBy {
166+
if i > 0 {
167+
f.builder.WriteString(", ")
168+
}
169+
if orderBy.Expression != nil {
170+
if err := f.formatExpression(orderBy.Expression); err != nil {
171+
fmt.Fprintf(os.Stderr, "Warning: failed to format ORDER BY expression: %v\n", err)
172+
}
173+
}
174+
// Add ASC/DESC
175+
if !orderBy.Ascending {
176+
f.builder.WriteString(" DESC")
177+
}
178+
// Add NULLS FIRST/LAST if specified
179+
if orderBy.NullsFirst != nil {
180+
if *orderBy.NullsFirst {
181+
f.builder.WriteString(" NULLS FIRST")
182+
} else {
183+
f.builder.WriteString(" NULLS LAST")
184+
}
185+
}
186+
}
166187
}
167188

168189
// LIMIT clause
@@ -537,7 +558,26 @@ func (f *SQLFormatter) formatWindowSpec(spec *ast.WindowSpec) error {
537558
}
538559
f.writeKeyword("ORDER BY")
539560
f.builder.WriteString(" ")
540-
f.formatExpressionList(spec.OrderBy, ", ")
561+
for i, orderBy := range spec.OrderBy {
562+
if i > 0 {
563+
f.builder.WriteString(", ")
564+
}
565+
if orderBy.Expression != nil {
566+
if err := f.formatExpression(orderBy.Expression); err != nil {
567+
fmt.Fprintf(os.Stderr, "Warning: failed to format window ORDER BY expression: %v\n", err)
568+
}
569+
}
570+
if !orderBy.Ascending {
571+
f.builder.WriteString(" DESC")
572+
}
573+
if orderBy.NullsFirst != nil {
574+
if *orderBy.NullsFirst {
575+
f.builder.WriteString(" NULLS FIRST")
576+
} else {
577+
f.builder.WriteString(" NULLS LAST")
578+
}
579+
}
580+
}
541581
}
542582

543583
if spec.FrameClause != nil {

‎pkg/gosqlx/extract.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -514,7 +514,9 @@ func (cc *columnCollector) collectFromNode(node ast.Node) {
514514
cc.collectFromExpression(n.Having)
515515
}
516516
for _, ob := range n.OrderBy {
517-
cc.collectFromExpression(ob)
517+
if ob.Expression != nil {
518+
cc.collectFromExpression(ob.Expression)
519+
}
518520
}
519521
if n.With != nil {
520522
cc.collectFromNode(n.With)
@@ -670,7 +672,9 @@ func (qcc *qualifiedColumnCollector) collectFromNode(node ast.Node) {
670672
qcc.collectFromExpression(n.Having)
671673
}
672674
for _, ob := range n.OrderBy {
673-
qcc.collectFromExpression(ob)
675+
if ob.Expression != nil {
676+
qcc.collectFromExpression(ob.Expression)
677+
}
674678
}
675679
if n.With != nil {
676680
qcc.collectFromNode(n.With)
@@ -833,7 +837,9 @@ func (fc *functionCollector) collectFromNode(node ast.Node) {
833837
fc.collectFromExpression(n.Having)
834838
}
835839
for _, ob := range n.OrderBy {
836-
fc.collectFromExpression(ob)
840+
if ob.Expression != nil {
841+
fc.collectFromExpression(ob.Expression)
842+
}
837843
}
838844
if n.With != nil {
839845
fc.collectFromNode(n.With)

‎pkg/sql/ast/ast.go‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -113,11 +113,27 @@ func (t *TableReference) statementNode() {}
113113
func (t TableReference) TokenLiteral() string { return t.Name }
114114
func (t TableReference) Children() []Node { return nil }
115115

116+
// OrderByExpression represents an ORDER BY clause element with direction and NULL ordering
117+
type OrderByExpression struct {
118+
Expression Expression // The expression to order by
119+
Ascending bool // true for ASC (default), false for DESC
120+
NullsFirst *bool // nil = default behavior, true = NULLS FIRST, false = NULLS LAST
121+
}
122+
123+
func (*OrderByExpression) expressionNode() {}
124+
func (o *OrderByExpression) TokenLiteral() string { return "ORDER BY" }
125+
func (o *OrderByExpression) Children() []Node {
126+
if o.Expression != nil {
127+
return []Node{o.Expression}
128+
}
129+
return nil
130+
}
131+
116132
// WindowSpec represents a window specification
117133
type WindowSpec struct {
118134
Name string
119135
PartitionBy []Expression
120-
OrderBy []Expression
136+
OrderBy []OrderByExpression
121137
FrameClause *WindowFrame
122138
}
123139

@@ -126,7 +142,10 @@ func (w WindowSpec) TokenLiteral() string { return "WINDOW" }
126142
func (w WindowSpec) Children() []Node {
127143
children := make([]Node, 0)
128144
children = append(children, nodifyExpressions(w.PartitionBy)...)
129-
children = append(children, nodifyExpressions(w.OrderBy)...)
145+
for _, orderBy := range w.OrderBy {
146+
orderBy := orderBy // G601: Create local copy to avoid memory aliasing
147+
children = append(children, &orderBy)
148+
}
130149
if w.FrameClause != nil {
131150
children = append(children, w.FrameClause)
132151
}
@@ -162,7 +181,7 @@ type SelectStatement struct {
162181
GroupBy []Expression
163182
Having Expression
164183
Windows []WindowSpec
165-
OrderBy []Expression
184+
OrderBy []OrderByExpression
166185
Limit *int
167186
Offset *int
168187
}
@@ -195,7 +214,10 @@ func (s SelectStatement) Children() []Node {
195214
window := window // G601: Create local copy to avoid memory aliasing
196215
children = append(children, &window)
197216
}
198-
children = append(children, nodifyExpressions(s.OrderBy)...)
217+
for _, orderBy := range s.OrderBy {
218+
orderBy := orderBy // G601: Create local copy to avoid memory aliasing
219+
children = append(children, &orderBy)
220+
}
199221
return children
200222
}
201223

‎pkg/sql/ast/dml.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ type Select struct {
88
Where Expression
99
GroupBy []Expression
1010
Having Expression
11-
OrderBy []Expression
11+
OrderBy []OrderByExpression
1212
Limit *int64
1313
Offset *int64
1414
}
@@ -29,7 +29,10 @@ func (s Select) Children() []Node {
2929
if s.Having != nil {
3030
children = append(children, s.Having)
3131
}
32-
children = append(children, nodifyExpressions(s.OrderBy)...)
32+
for _, orderBy := range s.OrderBy {
33+
orderBy := orderBy // G601: Create local copy to avoid memory aliasing
34+
children = append(children, &orderBy)
35+
}
3336
return children
3437
}
3538

‎pkg/sql/ast/dml_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func TestSelect(t *testing.T) {
7575
From: []TableReference{
7676
{Name: "users"},
7777
},
78-
OrderBy: []Expression{&Identifier{Name: "created_at"}},
78+
OrderBy: []OrderByExpression{{Expression: &Identifier{Name: "created_at"}, Ascending: true}},
7979
},
8080
wantLiteral: "SELECT",
8181
minChildren: 3,

‎pkg/sql/ast/nodes_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ func TestWindowSpec(t *testing.T) {
343343
windowSpec: &WindowSpec{
344344
Name: "w",
345345
PartitionBy: []Expression{&Identifier{Name: "dept"}},
346-
OrderBy: []Expression{&Identifier{Name: "salary"}},
346+
OrderBy: []OrderByExpression{{Expression: &Identifier{Name: "salary"}, Ascending: true}},
347347
},
348348
wantLiteral: "WINDOW",
349349
wantChildren: 2,

‎pkg/sql/ast/pool.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ var (
1919
New: func() interface{} {
2020
return &SelectStatement{
2121
Columns: make([]Expression, 0, 4),
22-
OrderBy: make([]Expression, 0, 1),
22+
OrderBy: make([]OrderByExpression, 0, 1),
2323
}
2424
},
2525
}
@@ -237,8 +237,10 @@ func PutSelectStatement(stmt *SelectStatement) {
237237
for _, col := range stmt.Columns {
238238
PutExpression(col)
239239
}
240-
for _, expr := range stmt.OrderBy {
241-
PutExpression(expr)
240+
for _, orderBy := range stmt.OrderBy {
241+
if orderBy.Expression != nil {
242+
PutExpression(orderBy.Expression)
243+
}
242244
}
243245
if stmt.Where != nil {
244246
PutExpression(stmt.Where)

0 commit comments

Comments
 (0)