Skip to content

Revert D96106074 #27373

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.

Sign up for GitHub

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

Closed
shelton408 wants to merge 1 commit into from
+7 −47
Closed

Revert D96106074 #27373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 6 additions & 21 deletions presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2967,10 +2967,7 @@ protected Scope visitQuerySpecification(QuerySpecification node, Optional<Scope>
analysis.setOrderByExpressions(node, orderByExpressions);

List<Expression> sourceExpressions = new ArrayList<>(outputExpressions);
// Use the rewritten HAVING expression (to resolve SELECT alias references)
if (node.getHaving().isPresent()) {
sourceExpressions.add(analysis.getHaving(node));
}
node.getHaving().ifPresent(sourceExpressions::add);

analyzeGroupingOperations(node, sourceExpressions, orderByExpressions);
List<FunctionCall> aggregates = analyzeAggregations(node, sourceExpressions, orderByExpressions);
Expand Down Expand Up @@ -3823,12 +3820,7 @@ private void analyzeHaving(QuerySpecification node, Scope scope)
if (node.getHaving().isPresent()) {
Expression predicate = node.getHaving().get();

// Reuse OrderByExpressionRewriter to resolve SELECT aliases in HAVING
Multimap<QualifiedName, Expression> namedOutputExpressions = extractNamedOutputExpressions(node.getSelect());
Expression rewrittenPredicate = ExpressionTreeRewriter.rewriteWith(new OrderByExpressionRewriter(namedOutputExpressions, "HAVING"), predicate);

// Analyze the rewritten expression
ExpressionAnalysis expressionAnalysis = analyzeExpression(rewrittenPredicate, scope);
ExpressionAnalysis expressionAnalysis = analyzeExpression(predicate, scope);

expressionAnalysis.getWindowFunctions().stream()
.findFirst()
Expand All @@ -3838,12 +3830,12 @@ private void analyzeHaving(QuerySpecification node, Scope scope)

analysis.recordSubqueries(node, expressionAnalysis);

Type predicateType = expressionAnalysis.getType(rewrittenPredicate);
Type predicateType = expressionAnalysis.getType(predicate);
if (!predicateType.equals(BOOLEAN) && !predicateType.equals(UNKNOWN)) {
throw new SemanticException(TYPE_MISMATCH, rewrittenPredicate, "HAVING clause must evaluate to a boolean: actual type %s", predicateType);
throw new SemanticException(TYPE_MISMATCH, predicate, "HAVING clause must evaluate to a boolean: actual type %s", predicateType);
}

analysis.setHaving(node, rewrittenPredicate);
analysis.setHaving(node, predicate);
}
}

Expand Down Expand Up @@ -3894,17 +3886,10 @@ private class OrderByExpressionRewriter
extends ExpressionRewriter<Void>
{
private final Multimap<QualifiedName, Expression> assignments;
private final String clauseName;

public OrderByExpressionRewriter(Multimap<QualifiedName, Expression> assignments)
{
this(assignments, "ORDER BY");
}

public OrderByExpressionRewriter(Multimap<QualifiedName, Expression> assignments, String clauseName)
{
this.assignments = assignments;
this.clauseName = clauseName;
}

@Override
Expand All @@ -3917,7 +3902,7 @@ public Expression rewriteIdentifier(Identifier reference, Void context, Expressi
.collect(Collectors.toSet());

if (expressions.size() > 1) {
throw new SemanticException(AMBIGUOUS_ATTRIBUTE, reference, "'%s' in '%s' is ambiguous", name, clauseName);
throw new SemanticException(AMBIGUOUS_ATTRIBUTE, reference, "'%s' in ORDER BY is ambiguous", name);
}

if (expressions.size() == 1) {
Expand Down
27 changes: 1 addition & 26 deletions presto-main-base/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,32 +308,7 @@ public void testReferenceToOutputColumnFromOrderByAggregation()
@Test
public void testHavingReferencesOutputAlias()
{
// HAVING now support referencing SELECT aliases for improved SQL compatibility
analyze("SELECT sum(a) x FROM t1 HAVING x > 5");
analyze("SELECT sum(a) AS total FROM t1 GROUP BY b HAVING total > 10");
analyze("SELECT count(*) AS cnt, sum(a) AS total FROM t1 GROUP BY b HAVING cnt > 5 AND total > 100");
analyze("SELECT sum(a) as sum_a FROM t1 GROUP BY b HAVING sum_a > 1");
}

@Test
public void testHavingAmbiguousAlias()
{
// Ambiguous alias referenced in HAVING should throw appropriate error
assertFails(AMBIGUOUS_ATTRIBUTE, "SELECT sum(a) AS x, count(b) AS x FROM t1 GROUP BY c HAVING x > 5");
}

@Test
public void testHavingNonExistentAlias()
{
// Non-existent alias in HAVING should fail with MISSING_ATTRIBUTE
assertFails(MISSING_ATTRIBUTE, "SELECT sum(a) AS total FROM t1 GROUP BY b HAVING unknown_alias > 5");
}

@Test
public void testHavingWindowFunctionViaAlias()
{
// Window functions are not allowed in HAVING, even when referenced via alias
assertFails(NESTED_WINDOW, "SELECT row_number() OVER () AS rn FROM t1 GROUP BY b HAVING rn > 1");
assertFails(MISSING_ATTRIBUTE, "SELECT sum(a) x FROM t1 HAVING x > 5");
}

@Test
Expand Down
Loading