Skip to content
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

[CALCITE-6337] Distinguish naked measure inside and outside aggregation #3738

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ boolean isMeasureExp(SqlNode e) {
return null;
}

if (!validator.config().nakedMeasures()
if (!validator.config().nakedMeasuresInAggregateQuery()
&& isMeasureExp(id)) {
SqlNode originalExpr = validator.getOriginal(id);
throw validator.newValidationError(originalExpr,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -912,15 +912,39 @@ interface Config {
*/
Config withLenientOperatorLookup(boolean lenient);

/** Returns whether the validator allows measures to be used without the
* AGGREGATE function. Default is true. */
@Value.Default default boolean nakedMeasures() {
/** Returns whether the validator allows measures to be used without
* AGGREGATE function in a non-aggregate query. Default is true.
*/
barrkel marked this conversation as resolved.
Show resolved Hide resolved
@Value.Default default boolean nakedMeasuresInNonAggregateQuery() {
return true;
}

/** Sets whether the validator allows measures to be used without AGGREGATE
* function in a non-aggregate query.
*/
Config withNakedMeasuresInNonAggregateQuery(boolean value);

/** Returns whether the validator allows measures to be used without
* AGGREGATE function in an aggregate query. Default is true.
*/
@Value.Default default boolean nakedMeasuresInAggregateQuery() {
return true;
}

/** Sets whether the validator allows measures to be used without AGGREGATE
* function in an aggregate query.
*/
Config withNakedMeasuresInAggregateQuery(boolean value);

/** Sets whether the validator allows measures to be used without the
* AGGREGATE function. */
Config withNakedMeasures(boolean nakedMeasures);
* AGGREGATE function inside or outside aggregate queries.
* Deprecated: use the inside / outside variants instead.
*/
@Deprecated // to be removed before 1.38
default Config withNakedMeasures(boolean nakedMeasures) {
barrkel marked this conversation as resolved.
Show resolved Hide resolved
return withNakedMeasuresInAggregateQuery(nakedMeasures)
.withNakedMeasuresInNonAggregateQuery(nakedMeasures);
}

/** Returns whether the validator supports implicit type coercion. */
@Value.Default default boolean typeCoercionEnabled() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2852,7 +2852,7 @@ private void registerQuery(
}
}

// If this is an aggregating query, the SELECT list and HAVING
// If this is an aggregate query, the SELECT list and HAVING
// clause use a different scope, where you can only reference
// columns which are in the GROUP BY clause.
SqlValidatorScope aggScope = selectScope;
Expand Down Expand Up @@ -2888,7 +2888,7 @@ private void registerQuery(
registerSubQueries(orderScope, orderList);

if (!isAggregate(select)) {
// Since this is not an aggregating query,
// Since this is not an aggregate query,
// there cannot be any aggregates in the ORDER BY clause.
SqlNode agg = aggFinder.findAgg(orderList);
if (agg != null) {
Expand Down Expand Up @@ -4826,10 +4826,10 @@ private void validateExpr(SqlNode expr, SqlValidatorScope scope) {
}
}

// Unless 'naked measures' are enabled, a non-aggregating query cannot
// reference measure columns. (An aggregating query can use them as
// Unless 'naked measures' are enabled, a non-aggregate query cannot
// reference measure columns. (An aggregate query can use them as
// argument to the AGGREGATE function.)
if (!config.nakedMeasures()
if (!config.nakedMeasuresInNonAggregateQuery()
&& !(scope instanceof AggregatingScope)
&& scope.isMeasureRef(expr)) {
throw newValidationError(expr,
Expand Down
37 changes: 29 additions & 8 deletions core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2866,7 +2866,7 @@ void testWinPartClause() {
.fails("OVER clause is necessary for window functions");

// With [CALCITE-1340], the validator would see RANK without OVER,
// mistakenly think this is an aggregating query, and wrongly complain
// mistakenly think this is an aggregate query, and wrongly complain
// about the PARTITION BY: "Expression 'DEPTNO' is not being grouped"
winSql("select cume_dist() over w , ^rank()^\n"
+ "from emp\n"
Expand Down Expand Up @@ -3874,8 +3874,19 @@ private void checkNegWindow(String s, String msg) {
SqlValidatorFixture f =
fixture().withExtendedCatalog()
.withOperatorTable(operatorTableFor(SqlLibrary.CALCITE));
SqlValidatorFixture f2 =
f.withValidatorConfig(c -> c.withNakedMeasures(false));

SqlValidatorFixture fNakedInsideOnly =
f.withValidatorConfig(c ->
c.withNakedMeasuresInAggregateQuery(true)
.withNakedMeasuresInNonAggregateQuery(false));
SqlValidatorFixture fNakedOutsideOnly =
f.withValidatorConfig(c ->
c.withNakedMeasuresInNonAggregateQuery(true)
.withNakedMeasuresInAggregateQuery(false));
SqlValidatorFixture fNoNakedMeasures =
f.withValidatorConfig(c ->
c.withNakedMeasuresInNonAggregateQuery(false)
.withNakedMeasuresInAggregateQuery(false));

final String measureIllegal =
"Measure expressions can only occur within AGGREGATE function";
Expand All @@ -3890,28 +3901,36 @@ private void checkNegWindow(String s, String msg) {
.ok();

// Same SQL is invalid if naked measures are not enabled
f2.withSql(sql0).fails(measureIllegal);
fNoNakedMeasures.withSql(sql0).fails(measureIllegal);
fNakedOutsideOnly.withSql(sql0).fails(measureIllegal);
fNakedInsideOnly.withSql(sql0).isAggregate(is(true)).ok();

// Similarly, with alias
final String sql1b = "select deptno, ^count_plus_100^ as x\n"
+ "from empm\n"
+ "group by deptno";
f.withSql(sql1b).isAggregate(is(true)).ok();
f2.withSql(sql1b).fails(measureIllegal);
fNoNakedMeasures.withSql(sql1b).fails(measureIllegal);
fNakedOutsideOnly.withSql(sql1b).fails(measureIllegal);
fNakedInsideOnly.withSql(sql1b).isAggregate(is(true)).ok();

// Similarly, in an expression
final String sql1c = "select deptno, deptno + ^count_plus_100^ * 2 as x\n"
+ "from empm\n"
+ "group by deptno";
f.withSql(sql1c).isAggregate(is(true)).ok();
f2.withSql(sql1c).fails(measureIllegal);
fNoNakedMeasures.withSql(sql1c).fails(measureIllegal);
fNakedOutsideOnly.withSql(sql1c).fails(measureIllegal);
fNakedInsideOnly.withSql(sql1c).isAggregate(is(true)).ok();

// Similarly, for a query that is an aggregate query because of another
// aggregate function.
final String sql1 = "select count(*), ^count_plus_100^\n"
+ "from empm";
f.withSql(sql1).isAggregate(is(true)).ok();
f2.withSql(sql1).fails(measureIllegal);
fNoNakedMeasures.withSql(sql1).fails(measureIllegal);
fNakedInsideOnly.withSql(sql1).isAggregate(is(true)).ok();
fNakedOutsideOnly.withSql(sql1).fails(measureIllegal);

// A measure in a non-aggregate query.
// Using a measure should not make it an aggregate query.
Expand All @@ -3924,7 +3943,9 @@ private void checkNegWindow(String s, String msg) {
+ "MEASURE<INTEGER NOT NULL> NOT NULL COUNT_PLUS_100, "
+ "VARCHAR(20) NOT NULL ENAME) NOT NULL")
.isAggregate(is(false));
f2.withSql(sql2).fails(measureIllegal2);
fNoNakedMeasures.withSql(sql2).fails(measureIllegal2);
fNakedInsideOnly.withSql(sql2).fails(measureIllegal2);
fNakedOutsideOnly.withSql(sql2).isAggregate(is(false)).ok();

// as above, wrapping the measure in AGGREGATE
final String sql3 = "select deptno, aggregate(count_plus_100) as x, ename\n"
Expand Down