Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,8 @@ public abstract class SqlAbstractConformance implements SqlConformance {
@Override public boolean isDistinctOnAllowed() {
return SqlConformanceEnum.DEFAULT.isDistinctOnAllowed();
}

@Override public boolean isCorrelatedAggregateAllowed() {
return SqlConformanceEnum.DEFAULT.isCorrelatedAggregateAllowed();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -691,4 +691,20 @@ default boolean isColonFieldAccessAllowed() {
* false otherwise.
*/
boolean isDistinctOnAllowed();

/**
* Whether an aggregate function inside a scalar sub-query is allowed to
* reference columns from an outer query.
*
* <p>This is not allowed by the SQL standard, but is supported by some
* databases, including SQLite, DuckDB and SQL Server.
*
* <p>Among the built-in conformance levels, true in
* {@link SqlConformanceEnum#BABEL},
* {@link SqlConformanceEnum#LENIENT};
* false otherwise.
*/
default boolean isCorrelatedAggregateAllowed() {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -542,4 +542,14 @@ public enum SqlConformanceEnum implements SqlConformance {
return false;
}
}

@Override public boolean isCorrelatedAggregateAllowed() {
switch (this) {
case BABEL:
case LENIENT:
return true;
default:
return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -181,4 +181,8 @@ protected SqlDelegatingConformance(SqlConformance delegate) {
@Override public boolean isDistinctOnAllowed() {
return delegate.isDistinctOnAllowed();
}

@Override public boolean isCorrelatedAggregateAllowed() {
return delegate.isCorrelatedAggregateAllowed();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@
import java.util.Objects;
import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.BiFunction;
import java.util.function.Consumer;
import java.util.function.Supplier;
Expand Down Expand Up @@ -5599,6 +5601,14 @@
// First pass, ensure that aliases are unique. "*" and "TABLE.*" items
// are ignored.

// Rewrite scalar sub-queries whose single select item is an aggregate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be done after validating all arguments?
Then you would not need to call deriveType early.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and this is the root cause behind the first question you asked this time.
The sub-query's SELECT scope is already established during the registration phase (registerSubQueries for the SELECT list), so the early deriveType calls were redundant. I've removed them; getSelectScope(subQuery) works directly.

// over outer columns. The aggregate belongs to the outer query per SQL
// standard, but only if the current conformance allows this non-standard
// correlated-aggregate construct.
if (config.conformance().isCorrelatedAggregateAllowed()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is an error reported if the conformance does not allow this?

@xuzifu666 xuzifu666 Jun 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reminder, I added handling and tests for cases where conformance is not allow, and also implemented custom exception messages for these scenarios. PTAL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read my review comments carefully. I had already suggested error handling in the previous iteration.
My energy for reviews is limited. This is not the only thing that could have been done in fewer steps.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, and thank you for pointing this out. You are right — I should have caught the error-handling requirement from your earlier comment instead of needing a second reminder.
I will be more careful reading review comments in full going forwar.

rewriteOuterAggregatesInSelectList(select);
}

// Validate SELECT list. Expand terms of the form "*" or "TABLE.*".
final SqlValidatorScope selectScope = getSelectScope(select);
final List<SqlNode> expandedSelectItems = new ArrayList<>();
Expand Down Expand Up @@ -5658,6 +5668,279 @@
return typeFactory.createStructType(fieldList);
}

/**
* Rewrites scalar sub-queries in the SELECT list whose single select item is
* an aggregate function whose arguments reference only outer columns. Per the
* SQL standard, such aggregates belong to the outer query.
*
* <p>The algorithm is:
* <ol>
* <li>For each item in the SELECT list, check whether it is a scalar
* sub-query, optionally wrapped in {@code AS} or {@code WITH}.
* <li>Inside the sub-query, require the SELECT list to contain exactly one
* item, and that item to be an aggregate function call.
* <li>Require every argument of the aggregate to reference only columns from
* the outer query (no inner columns), and to contain no nested sub-queries.
* <li>If all conditions hold, lift the aggregate out of the sub-query: keep
* the aggregate in the outer SELECT list, and replace the original sub-query
* with {@code (SELECT 1 FROM ... LIMIT 1)}. The result is equivalent because
* the scalar sub-query contributes a factor of one per outer row, while the
* aggregate is evaluated over the outer rows.
* <li>This rewrite will only work for numeric aggregates on types which
* have a multiplication operation.
* <li>If the outer query becomes an aggregate query as a result, upgrade its
* SELECT clause scope from {@link SelectScope} to
* {@link AggregatingSelectScope}.
* </ol>
*
* <p>For example,
* <blockquote><pre>
* WITH aa(a) AS (VALUES 1, 2, 3),
* t(x) AS (VALUES 10, 20, 30)
* SELECT (SELECT sum(a) FROM t) FROM aa
* </pre></blockquote>
* is rewritten to
* <blockquote><pre>
* WITH aa(a) AS (VALUES 1, 2, 3),
* t(x) AS (VALUES 10, 20, 30)
* SELECT sum(a) * (SELECT 1 FROM t LIMIT 1) FROM aa

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rewrite will only work for numeric aggregates on types which have a multiplication operation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this message to the comments first.

* </pre></blockquote>
*/
private void rewriteOuterAggregatesInSelectList(SqlSelect select) {
final SqlNodeList selectItems = select.getSelectList();
if (selectItems == null) {
return;
}
final SqlValidatorScope selectScope = getSelectScope(select);
final boolean wasAggregate = isAggregate(select);
for (int i = 0; i < selectItems.size(); i++) {
final SqlNode selectItem = selectItems.get(i);
final SqlNode rewrittenItem =
rewriteOuterAggregateItem(selectScope, selectItem);
if (rewrittenItem != selectItem) {
selectItems.set(i, rewrittenItem);
}
}
// The rewrite may have introduced an aggregate into a query that was not
// previously aggregate. Update the SELECT clause scope accordingly so that
// subsequent validation and conversion see an AggregatingSelectScope.
if (!wasAggregate && isAggregate(select)) {
SqlValidatorScope scope =
clauseScopes.get(IdPair.of(select, Clause.SELECT));
if (scope instanceof AggregatingSelectScope) {
scope = ((AggregatingSelectScope) scope).getParent();
}
clauseScopes.put(IdPair.of(select, Clause.SELECT),
new AggregatingSelectScope(
requireNonNull(scope, "scope"), select, false));
}
}

/**
* Rewrites a single SELECT list item if it is a scalar sub-query containing
* an aggregate over outer columns.
*
* @return the rewritten expression, or {@code selectItem} if no rewrite applies
*/
private SqlNode rewriteOuterAggregateItem(SqlValidatorScope parentScope,
SqlNode selectItem) {
// Unwrap AS.
SqlNode expr = selectItem;
@Nullable SqlIdentifier alias = null;
if (SqlUtil.isCallTo(selectItem, SqlStdOperatorTable.AS)) {
SqlCall asCall = (SqlCall) selectItem;
expr = asCall.operand(0);
alias = asCall.operand(1);
}
if (!SqlUtil.isCallTo(expr, SqlStdOperatorTable.SCALAR_QUERY)) {
return selectItem;
}
final SqlBasicCall scalarSubQuery = (SqlBasicCall) expr;
final SqlNode query = scalarSubQuery.operand(0);
final SqlSelect subQuery = query instanceof SqlWith
? (SqlSelect) ((SqlWith) query).body
: query instanceof SqlSelect
? (SqlSelect) query
: null;

Check warning on line 5764 in core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Extract this nested ternary operation into an independent statement.

See more on https://sonarcloud.io/project/issues?id=apache_calcite&issues=AZ8E5Gg8VhkxC5OQBjm6&open=AZ8E5Gg8VhkxC5OQBjm6&pullRequest=5052
if (subQuery == null) {
return selectItem;
}
// Validate the sub-query first so that its scope is established.
deriveType(parentScope, scalarSubQuery);
final SqlNode rewrittenSubQuery =
rewriteOuterAggregate(query, subQuery, parentScope);
if (rewrittenSubQuery == query) {
return selectItem;
}
SqlNode result = rewrittenSubQuery;
if (alias != null) {
result =
SqlStdOperatorTable.AS.createCall(selectItem.getParserPosition(),
rewrittenSubQuery, alias);
}
return result;
}

/**
* Rewrites a scalar sub-query whose single select item is an aggregate
* function whose arguments reference only outer columns. The aggregate is
* pulled out to the enclosing query, and the sub-query's select item is
* replaced with the constant 1.
*
* @return the rewritten sub-query expression, or {@code subQuery} if no rewrite applies
*/
private SqlNode rewriteOuterAggregate(SqlNode originalQuery,
SqlSelect subQuery, SqlValidatorScope parentScope) {
final SqlNodeList subSelectItems = SqlNonNullableAccessors.getSelectList(subQuery);
if (subSelectItems.size() != 1) {
return originalQuery;
}
if (subQuery.getGroup() != null && !subQuery.getGroup().isEmpty()) {
return originalQuery;
}
SqlNode subSelectItem = subSelectItems.get(0);
if (SqlUtil.isCallTo(subSelectItem, SqlStdOperatorTable.AS)) {
subSelectItem = ((SqlCall) subSelectItem).operand(0);
}
if (!(subSelectItem instanceof SqlCall)) {
return originalQuery;
}
final SqlCall aggCall = (SqlCall) subSelectItem;
if (!aggCall.getOperator().isAggregator()) {
return originalQuery;
}
final SqlValidatorScope subScope = getSelectScope(subQuery);
if (subScope == null) {
return originalQuery;
}
final SqlValidatorScope operandScope;
if (subScope instanceof AggregatingSelectScope) {
operandScope = ((AggregatingSelectScope) subScope).parent;
} else {
operandScope = subScope;
}
if (operandScope == null) {
return originalQuery;
}
for (SqlNode operand : aggCall.getOperandList()) {
if (!referencesOnlyOuterColumns(operand, parentScope, operandScope)) {
return originalQuery;
}
}
// Rewrite: replace aggregate with 1 in a new sub-query, and multiply
// by the aggregate in the outer query.
final SqlLiteral one = SqlLiteral.createExactNumeric("1", SqlParserPos.ZERO);
final SqlNodeList newSubSelectItems =
new SqlNodeList(ImmutableList.of(one), subSelectItems.getParserPosition());
final SqlNodeList keywordList = (SqlNodeList) subQuery.getOperandList().get(0);
final SqlSelect newSubQuery =
new SqlSelect(subQuery.getParserPosition(),
keywordList,
newSubSelectItems,
subQuery.getFrom(),
subQuery.getWhere(),
subQuery.getGroup(),
subQuery.getHaving(),
subQuery.getWindowList(),
subQuery.getQualify(),
subQuery.getOrderList(),
subQuery.getOffset(),
subQuery.getFetch(),
subQuery.getHints(),
subQuery.getDistinctOn());
// Ensure the rewritten sub-query still returns at most one row, because
// removing the aggregate may otherwise produce multiple rows.
if (newSubQuery.getFetch() == null) {
newSubQuery.setFetch(
SqlLiteral.createExactNumeric("1", SqlParserPos.ZERO));
}
final SqlNode newQuery = originalQuery instanceof SqlWith
? new SqlWith(originalQuery.getParserPosition(),
((SqlWith) originalQuery).withList, newSubQuery)
: newSubQuery;
registerQuery(parentScope, null, newQuery, newQuery, null, false);
validateQuery(newQuery, parentScope, unknownType);
final SqlNode scalarSubQuery =
SqlStdOperatorTable.SCALAR_QUERY.createCall(newQuery.getParserPosition(), newQuery);
return SqlStdOperatorTable.MULTIPLY.createCall(
aggCall.getParserPosition(), aggCall, scalarSubQuery);
}

/**
* Returns whether an expression contains only references to columns that are
* outside the current select scope, and can be resolved in the parent scope.
*/
private boolean referencesOnlyOuterColumns(SqlNode node,
SqlValidatorScope parentScope, SqlValidatorScope currentScope) {
// Do not rewrite if the aggregate argument contains a sub-query; the
// sub-query may reference inner columns, and pulling the aggregate out
// would change the semantics.
if (containsSubQuery(node)) {
return false;
}
final AtomicBoolean ok = new AtomicBoolean(true);
final AtomicInteger outerColumnCount = new AtomicInteger(0);
node.accept(new SqlBasicVisitor<Void>() {
@Override public Void visit(SqlIdentifier id) {
if (!isOuterReference(currentScope, id)) {
ok.set(false);
return null;
}
// The identifier must be resolvable in the parent scope; otherwise it
// references an intermediate scope, not the immediate enclosing query.
try {
parentScope.fullyQualify(id);
outerColumnCount.incrementAndGet();
} catch (Exception e) {
ok.set(false);
}
return null;
}
});
return ok.get() && outerColumnCount.get() > 0;
}

/**
* Returns whether an expression contains a sub-query.
*/
private static boolean containsSubQuery(SqlNode node) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there no other helper function doing this in the Calcite codebase?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's RexUtil.SubQueryFinder, but it operates on the Rex layer (RexSubQuery), whereas here we're still at the SqlNode layer, so it doesn't apply. I didn't find an existing SqlNode-level "contains sub-query" helper. That said, the hand-written switch over every sub-query SqlKind is fragile (easy to miss a new kind) — I had replaced it with the predefined SqlKind.QUERY set (SqlKind.QUERY.contains(call.getKind())) instead of enumerating each case.

final AtomicBoolean found = new AtomicBoolean(false);
node.accept(new SqlBasicVisitor<Void>() {
@Override public Void visit(SqlCall call) {
switch (call.getKind()) {
case SCALAR_QUERY:
case EXISTS:
case IN:
case NOT_IN:
case SOME:
case ALL:
case MULTISET_QUERY_CONSTRUCTOR:
case ARRAY_QUERY_CONSTRUCTOR:
case MAP_QUERY_CONSTRUCTOR:
case CURSOR:
found.set(true);
return null;
default:
return super.visit(call);
}
}
});
return found.get();
}

/**
* Returns whether an identifier refers to a scope outside the current select.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure exactly what "outside" means, but given this function definition I think that the javadoc could be "in a scope which is not the supplied one". There's certainly no "current select" in sight.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense; I’ve made the relevant changes to the comment.

*/
private boolean isOuterReference(SqlValidatorScope scope, SqlIdentifier id) {
final SqlQualified fqId = scope.fullyQualify(id);
if (fqId.prefixLength <= 0) {
return false;
}
final SqlValidatorScope.ResolvedImpl resolved = new SqlValidatorScope.ResolvedImpl();
scope.resolve(fqId.prefix(), catalogReader.nameMatcher(), false, resolved);
return resolved.count() == 1 && !resolved.only().scope.isWithin(scope);
}

/**
* Validates an expression.
*
Expand Down
Loading
Loading