Skip to content

Commit

Permalink
[CALCITE-6337] Distinguish naked measure support between inside and o…
Browse files Browse the repository at this point in the history
…utside aggregation

Split nakedMeasures flag into two flags:
- nakedMeasuresInsideAggregatingQuery : allowed in group-by queries
- nakedMeasuresOutsideAggregatingQuery : allowed in non-group-by queries

The existing nakedMeasures flag is marked as deprecated. Its functionality
is equivalent to setting the above two flags to the same value.
  • Loading branch information
barrkel authored and tanclary committed Mar 24, 2024
1 parent 4823cb7 commit 14ade3a
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 19 deletions.
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.
*/
@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) {
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

0 comments on commit 14ade3a

Please sign in to comment.