feat: move ORDER BY from BinaryQuery right side to SimpleQuery level

root · claude · root · commit 96d298fef7e4 · 2025-07-28T15:55:44.000+09:00
Clarifies UNION ORDER BY semantics by extracting ORDER BY clause from the rightmost query in binary operations and moving it to the outer SimpleSelectQuery level. This transforms ambiguous ORDER BY positioning into explicit subquery structure. Changes: - Add ORDER BY extraction logic to QueryBuilder.buildSimpleBinaryQuery() - Implement recursive ORDER BY detection in binary query trees - Move ORDER BY from individual subqueries to wrapper SimpleQuery - Add comprehensive tests for ORDER BY movement scenarios - Document SQL standard behavior for UNION ORDER BY clauses 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
diff --git a/packages/core/src/transformers/QueryBuilder.ts b/packages/core/src/transformers/QueryBuilder.ts
@@ -1,4 +1,4 @@
-import { SetClause, SetClauseItem, FromClause, WhereClause, SelectClause, SelectItem, SourceAliasExpression, SourceExpression, SubQuerySource, WithClause, TableSource, UpdateClause, InsertClause } from '../models/Clause';
+import { SetClause, SetClauseItem, FromClause, WhereClause, SelectClause, SelectItem, SourceAliasExpression, SourceExpression, SubQuerySource, WithClause, TableSource, UpdateClause, InsertClause, OrderByClause } from '../models/Clause';
 import { UpdateQuery } from '../models/UpdateQuery';
 import { BinaryExpression, ColumnReference } from '../models/ValueComponent';
 import { SelectValueCollector } from './SelectValueCollector';
@@ -66,7 +66,10 @@ export class QueryBuilder {
     }
 
     private static buildSimpleBinaryQuery(query: BinarySelectQuery): SimpleSelectQuery {
-        // Create a subquery source from the binary query
+        // Extract ORDER BY from the rightmost query in the binary tree and remove it
+        const extractedOrderBy = QueryBuilder.extractAndRemoveOrderByFromBinaryQuery(query);
+        
+        // Create a subquery source from the binary query (now without ORDER BY)
         const subQuerySource = new SubQuerySource(query);
 
         // Create a source expression with alias
@@ -81,17 +84,65 @@ export class QueryBuilder {
         // Create SELECT clause with * (all columns)
         const selectClause = QueryBuilder.createSelectAllClause();
 
-        // Create the final simple select query
+        // Create the final simple select query with extracted ORDER BY
         const q = new SimpleSelectQuery(
             {
                 selectClause,
-                fromClause
+                fromClause,
+                orderByClause: extractedOrderBy
             }
         );
 
         return CTENormalizer.normalize(q) as SimpleSelectQuery;
     }
 
+    /**
+     * Extracts ORDER BY clause from the rightmost query in a binary query tree and removes it.
+     * This clarifies the semantics by moving the ORDER BY from the ambiguous position 
+     * in the UNION to the explicit outer SimpleSelectQuery level.
+     * 
+     * NOTE: ORDER BY in UNION context applies to the entire result set, not individual subqueries.
+     * Therefore, table prefixes (e.g., "a.column") in ORDER BY are invalid SQL and would cause 
+     * syntax errors. Valid ORDER BY clauses should only reference column names without prefixes
+     * or use positional notation (ORDER BY 1, 2). Since we only process valid SQL, the current
+     * implementation correctly handles legitimate cases without additional prefix processing.
+     * 
+     * @param query BinarySelectQuery to process
+     * @returns Extracted OrderByClause or null if none found
+     */
+    private static extractAndRemoveOrderByFromBinaryQuery(query: BinarySelectQuery): OrderByClause | null {
+        return QueryBuilder.findAndRemoveRightmostOrderBy(query);
+    }
+
+    /**
+     * Recursively finds and removes ORDER BY from the rightmost query in a binary tree.
+     * 
+     * @param query Current query being processed
+     * @returns Extracted OrderByClause or null
+     */
+    private static findAndRemoveRightmostOrderBy(query: SelectQuery): OrderByClause | null {
+        if (query instanceof BinarySelectQuery) {
+            // For binary queries, check right side first (rightmost takes precedence)
+            const rightOrderBy = QueryBuilder.findAndRemoveRightmostOrderBy(query.right);
+            if (rightOrderBy) {
+                return rightOrderBy;
+            }
+            
+            // If no ORDER BY on right side, check left side
+            return QueryBuilder.findAndRemoveRightmostOrderBy(query.left);
+        }
+        else if (query instanceof SimpleSelectQuery) {
+            // Extract ORDER BY from SimpleSelectQuery and remove it
+            const orderBy = query.orderByClause;
+            if (orderBy) {
+                query.orderByClause = null;
+                return orderBy;
+            }
+        }
+        
+        return null;
+    }
+
     /**
      * Converts a ValuesQuery to a SimpleSelectQuery with sequentially numbered columns or user-specified columns
      * 
diff --git a/packages/core/tests/models/SelectQuery.toSimpleQuery.test.ts b/packages/core/tests/models/SelectQuery.toSimpleQuery.test.ts
@@ -49,7 +49,7 @@ describe('SelectQuery toSimpleQuery() conversion', () => {
 
         test('should enable CTE management on converted BinarySelectQuery', () => {
             // Test that converted binary query supports CTE operations
-            const query1 = SelectQueryParser.parse('SELECT id FROM users');
+            const query1 = SelectQueryParser.parse('SELECT id FROM users').toSimpleQuery();
             const query2 = SelectQueryParser.parse('SELECT id FROM customers');
             const binaryQuery = query1.toUnion(query2);
             const cteQuery = SelectQueryParser.parse('SELECT id FROM accounts WHERE active = true');
@@ -65,7 +65,7 @@ describe('SelectQuery toSimpleQuery() conversion', () => {
 
         test('should produce valid SQL with complex UNION and CTE', () => {
             // Test full workflow: UNION -> toSimpleQuery -> addCTE -> format
-            const query1 = SelectQueryParser.parse('SELECT id, name FROM employees');
+            const query1 = SelectQueryParser.parse('SELECT id, name FROM employees').toSimpleQuery();
             const query2 = SelectQueryParser.parse('SELECT id, name FROM contractors');
             const binaryQuery = query1.toUnionAll(query2);
             const cteQuery = SelectQueryParser.parse('SELECT id FROM departments WHERE active = true');
@@ -84,7 +84,7 @@ describe('SelectQuery toSimpleQuery() conversion', () => {
     describe('Method chaining patterns', () => {
         test('should support fluent API: binary -> toSimpleQuery -> CTE operations', () => {
             // Test the recommended usage pattern
-            const query1 = SelectQueryParser.parse('SELECT id FROM table1');
+            const query1 = SelectQueryParser.parse('SELECT id FROM table1').toSimpleQuery();
             const query2 = SelectQueryParser.parse('SELECT id FROM table2');
             const cte1 = SelectQueryParser.parse('SELECT id FROM temp1');
             const cte2 = SelectQueryParser.parse('SELECT id FROM temp2');
@@ -100,6 +100,115 @@ describe('SelectQuery toSimpleQuery() conversion', () => {
         });
     });
 
+    describe('BinarySelectQuery ORDER BY handling', () => {
+        // NOTE: In SQL standard, ORDER BY in UNION context applies to the entire result set.
+        // Table prefixes (e.g., "a.column") would be invalid SQL syntax in ORDER BY clauses
+        // following UNION operations. Only column names without prefixes or positional notation
+        // (ORDER BY 1, 2) are valid. This implementation correctly handles valid SQL cases.
+        
+        test('should move ORDER BY from right query to SimpleQuery when converting', () => {
+            // Test ORDER BY removal from right query and movement to SimpleQuery
+            const query1 = SelectQueryParser.parse('SELECT id, name FROM users').toSimpleQuery();
+            const query2 = SelectQueryParser.parse('SELECT id, name FROM customers ORDER BY name ASC');
+            const binaryQuery = query1.toUnion(query2);
+            
+            const result = binaryQuery.toSimpleQuery();
+            const formatted = formatter.format(result);
+            
+            // ORDER BY should be at SimpleQuery level, not in the subquery
+            expect(result.orderByClause).not.toBeNull();
+            
+            // Verify ORDER BY is at the outermost level (after the subquery)
+            // Should be: SELECT * FROM (...) AS "bq" ORDER BY "name"
+            const sql = formatted.formattedSql.trim();
+            expect(sql).toMatch(/\)\s+as\s+"bq"\s+order\s+by\s+"name"/i);
+            
+            // The binary query itself should no longer have ORDER BY on the right side
+            const binaryFormatted = formatter.format(binaryQuery);
+            // Should not contain ORDER BY before UNION
+            expect(binaryFormatted.formattedSql).not.toMatch(/order\s+by\s+[^)]+\)\s*$/i);
+        });
+
+        test('should handle multiple ORDER BY clauses correctly', () => {
+            // Test when both queries have ORDER BY - should use right query's ORDER BY
+            const query1 = SelectQueryParser.parse('SELECT id, name FROM users ORDER BY id DESC').toSimpleQuery();
+            const query2 = SelectQueryParser.parse('SELECT id, name FROM customers ORDER BY name ASC, id DESC');
+            const binaryQuery = query1.toUnion(query2);
+            
+            const result = binaryQuery.toSimpleQuery();
+            const formatted = formatter.format(result);
+            
+            // Should have ORDER BY at SimpleQuery level from right query
+            expect(result.orderByClause).not.toBeNull();
+            
+            // Verify ORDER BY is moved to outermost level and contains columns from right query
+            const sql = formatted.formattedSql.trim();
+            expect(sql).toMatch(/\)\s+as\s+"bq"\s+order\s+by\s+"name"[^,]*,\s*"id"/i);
+            
+            // Original binary query should have ORDER BY removed from right side
+            const binaryFormatted = formatter.format(binaryQuery);
+            // Left query still has its ORDER BY (not removed)
+            expect(binaryFormatted.formattedSql).toContain('order by "id" desc union');
+            // But right query's ORDER BY should be removed
+            expect(binaryFormatted.formattedSql).not.toMatch(/customers.*order\s+by.*$/i);
+        });
+
+        test('should work with nested binary queries', () => {
+            // Test ORDER BY handling with nested binary operations
+            const query1 = SelectQueryParser.parse('SELECT id FROM table1').toSimpleQuery();
+            const query2 = SelectQueryParser.parse('SELECT id FROM table2 ORDER BY id ASC');
+            const query3 = SelectQueryParser.parse('SELECT id FROM table3');
+            
+            const binaryQuery1 = query1.toUnion(query2);
+            const binaryQuery2 = binaryQuery1.toSimpleQuery().toUnion(query3);
+            
+            const result = binaryQuery2.toSimpleQuery();
+            const formatted = formatter.format(result);
+            
+            // Should extract ORDER BY from the rightmost query that has it
+            expect(result.orderByClause).not.toBeNull();
+            
+            // ORDER BY should be at the outermost level
+            const sql = formatted.formattedSql.trim();
+            expect(sql).toMatch(/\)\s+as\s+"bq"\s+order\s+by\s+"id"/i);
+            
+            // Verify the nested structure doesn't have ORDER BY in the middle
+            expect(sql).not.toMatch(/table2.*order.*union/i);
+        });
+
+        test('should handle case with no ORDER BY in any query', () => {
+            // Test when no queries have ORDER BY
+            const query1 = SelectQueryParser.parse('SELECT id FROM users').toSimpleQuery();
+            const query2 = SelectQueryParser.parse('SELECT id FROM customers');
+            const binaryQuery = query1.toUnion(query2);
+            
+            const result = binaryQuery.toSimpleQuery();
+            
+            // Should have no ORDER BY clause
+            expect(result.orderByClause).toBeNull();
+        });
+
+        test('should verify ORDER BY is actually removed from original query', () => {
+            // Create queries and verify ORDER BY removal
+            const query1 = SelectQueryParser.parse('SELECT id FROM users').toSimpleQuery();
+            const query2 = SelectQueryParser.parse('SELECT id FROM customers ORDER BY id DESC').toSimpleQuery();
+            
+            // Store original ORDER BY state
+            expect(query2.orderByClause).not.toBeNull();
+            const originalOrderBy = query2.orderByClause;
+            
+            const binaryQuery = query1.toUnion(query2);
+            const result = binaryQuery.toSimpleQuery();
+            
+            // Original query should have ORDER BY removed
+            expect(query2.orderByClause).toBeNull();
+            
+            // Result should have the ORDER BY
+            expect(result.orderByClause).not.toBeNull();
+            expect(result.orderByClause).toBe(originalOrderBy);
+        });
+    });
+
     describe('ValuesQuery.toSimpleQuery()', () => {
         test('should convert ValuesQuery to SimpleSelectQuery', () => {
             // Test conversion of VALUES query
