-
Notifications
You must be signed in to change notification settings - Fork 66
Fix the ORDER BY does not resolve alias from SELECT statement. #1831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
d96aef1
temp working
xd1313113 09b55f0
working
xd1313113 5f70735
minor
xd1313113 ec584e1
add tests
xd1313113 6412350
add more tests
xd1313113 e5dea5c
minor
xd1313113 42db1de
minor
xd1313113 d9ef648
minor
xd1313113 b7740e6
minor
xd1313113 bca6a1a
minor
xd1313113 717727c
minor
xd1313113 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
179 changes: 179 additions & 0 deletions
179
...ql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/OrderByAliasSupport.kt
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,179 @@ | ||
/* | ||
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"). | ||
* You may not use this file except in compliance with the License. | ||
* A copy of the License is located at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* or in the "license" file accompanying this file. This file is distributed | ||
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
|
||
package org.partiql.planner.internal.transforms | ||
|
||
import org.partiql.ast.Ast.orderBy | ||
import org.partiql.ast.Ast.sort | ||
import org.partiql.ast.AstNode | ||
import org.partiql.ast.AstRewriter | ||
import org.partiql.ast.OrderBy | ||
import org.partiql.ast.QueryBody | ||
import org.partiql.ast.SelectItem | ||
import org.partiql.ast.Statement | ||
import org.partiql.ast.expr.Expr | ||
import org.partiql.ast.expr.ExprQuerySet | ||
import org.partiql.ast.expr.ExprVarRef | ||
import org.partiql.planner.internal.PErrors | ||
import org.partiql.spi.errors.PErrorListener | ||
import kotlin.collections.MutableMap | ||
|
||
/** | ||
* Normalizes ORDER BY expressions by replacing SELECT aliases with their original expressions. | ||
* Uses a stack-based approach to maintain separate alias maps for each query scope, | ||
* enabling proper alias resolution in nested queries and set operations. | ||
*/ | ||
internal class OrderByAliasSupport(val listener: PErrorListener) : AstPass { | ||
/** | ||
* Context for tracking parent query scopes and their alias mappings. | ||
* | ||
* @property parentStack Stack of ExprQuerySet nodes representing nested query scopes | ||
* @property aliasList Maps each query scope to its SELECT alias definitions | ||
*/ | ||
data class Context(val parentStack: ArrayDeque<ExprQuerySet> = ArrayDeque(), val aliasList: MutableMap<ExprQuerySet, MutableList<Pair<String, Expr>>> = mutableMapOf()) | ||
|
||
override fun apply(statement: Statement): Statement { | ||
return Visitor(listener).visitStatement(statement, Context()) as Statement | ||
} | ||
|
||
/** | ||
* AST visitor that uses a stack-based approach to track parent query scopes. | ||
* | ||
* Key behaviors: | ||
* - Each ExprQuerySet creates its own alias scope on the stack | ||
* - SELECT aliases are collected into the current scope's map | ||
* - ORDER BY expressions resolve aliases from the appropriate scope | ||
* - Set operations (UNION, INTERSECT, EXCEPT) are skipped Order-By alias replacement | ||
alancai98 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
* - Case sensitivity is handled for both regular and delimited identifiers | ||
* | ||
* Example with nested queries: | ||
* ```sql | ||
* SELECT pid AS p FROM ( | ||
* SELECT productId AS pid FROM products ORDER BY pid | ||
* ) ORDER BY p | ||
* ``` | ||
* | ||
* Stack operations: | ||
* 1. Push outer query scope, collect "p" -> pid | ||
* 2. Push inner query scope, collect "pid" -> productId | ||
* 3. Resolve ORDER BY pid using inner scope | ||
* 4. Pop inner scope | ||
* 5. Resolve ORDER BY p using outer scope | ||
* 6. Pop outer scope | ||
*/ | ||
private class Visitor(val listener: PErrorListener) : AstRewriter<Context>() { | ||
/** | ||
* Manages query scope stack for each ExprQuerySet. | ||
* Pushes current query to stack on entry, pops on exit to maintain proper nesting. | ||
*/ | ||
override fun visitExprQuerySet(node: ExprQuerySet, ctx: Context): AstNode { | ||
// Push current query scope onto stack | ||
ctx.parentStack.addLast(node) | ||
XuechunHHH marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
ctx.aliasList[node] = mutableListOf() | ||
|
||
val transformed = super.visitExprQuerySet(node, ctx) | ||
|
||
// Pop scope when exiting query | ||
ctx.parentStack.removeLast() | ||
return transformed | ||
} | ||
|
||
/** | ||
* Collects SELECT aliases into the current query scope's alias map. | ||
* Only processes SelectItem.Expr nodes that have AS aliases defined. | ||
*/ | ||
override fun visitSelectItem(node: SelectItem, ctx: Context): AstNode { | ||
xd1313113 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (node is SelectItem.Expr) { | ||
node.asAlias?.let { alias -> | ||
// Add alias mapping to current query scope | ||
ctx.aliasList[ctx.parentStack.last()]?.add(alias.text to node.expr) | ||
} | ||
} | ||
return node | ||
} | ||
|
||
/** | ||
* Resolves ORDER BY expressions by replacing aliases with their original expressions. | ||
* For set operations, skip alias resolvation | ||
*/ | ||
override fun visitOrderBy(node: OrderBy, ctx: Context): AstNode { | ||
val parent = ctx.parentStack.last() | ||
// Skip alias replacement if OrderBy belongs to set operator. | ||
if (parent.body is QueryBody.SetOp) { | ||
return node | ||
} | ||
|
||
// Regular queries use their own alias map | ||
val aliasMap = ctx.aliasList[parent]!! | ||
if (aliasMap.isEmpty()) return node | ||
|
||
val transformedSorts = node.sorts.map { sort -> | ||
val transformedExpr = resolveExpr(sort.expr, aliasMap) | ||
if (transformedExpr != sort.expr) { | ||
sort( | ||
expr = transformedExpr, | ||
order = sort.order, | ||
nulls = sort.nulls | ||
) | ||
} else { | ||
sort | ||
} | ||
} | ||
return orderBy(transformedSorts) | ||
} | ||
|
||
/** | ||
* Resolves expressions recursively, handling aliases and complex expressions. | ||
* | ||
* Case sensitivity rules: | ||
* - Regular identifiers (unquoted): case-insensitive matching | ||
* - Delimited identifiers (quoted): case-sensitive matching | ||
* | ||
* @param expr Expression to resolve | ||
* @param aliasMap Current scope's alias mappings | ||
* @return Resolved expression or original if no alias found | ||
*/ | ||
private fun resolveExpr(expr: Expr, aliasMap: List<Pair<String, Expr>>): Expr { | ||
return when (expr) { | ||
is ExprVarRef -> { | ||
val identifier = expr.identifier.identifier | ||
val orderByName = identifier.text | ||
val isOrderByRegular = identifier.isRegular | ||
|
||
// Find matching alias considering case sensitivity | ||
val candidates = aliasMap.filter { orderByName.equals(it.first, ignoreCase = isOrderByRegular) } | ||
xd1313113 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
if (candidates.size == 1) { | ||
candidates[0].second | ||
} else { | ||
if (candidates.size > 1) { | ||
val candidateNames = candidates.map { | ||
val ref = it.second | ||
if (ref is ExprVarRef) { | ||
ref.identifier.identifier.text | ||
} else { | ||
"Not a column name or alias" | ||
xd1313113 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
} | ||
}.toList() | ||
xd1313113 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
listener.report(PErrors.varRefAmbiguous(null, expr.identifier, candidateNames)) | ||
} | ||
expr | ||
} | ||
} | ||
else -> expr | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.