mihaibudiu commented on code in PR #3837:
URL: https://github.com/apache/calcite/pull/3837#discussion_r1661339763
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectMergeRule.java:
##########
@@ -174,8 +174,9 @@ public interface Config extends RelRule.Config {
/** Defines an operand tree for the given classes. */
default Config withOperandFor(Class<? extends Project> projectClass) {
return withOperandSupplier(b0 ->
- b0.operand(projectClass).oneInput(b1 ->
- b1.operand(projectClass).anyInputs()))
+ b0.operand(projectClass)
Review Comment:
this seems to be unchanged
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2261,7 +2261,8 @@ where extract(YEAR from "sqlTimestamp") IN (1969, 1970);
!ok
-!if (false) {
+!use scott
Review Comment:
these don't really look related to the measures PR
##########
core/src/main/java/org/apache/calcite/rel/RelNodes.java:
##########
@@ -49,6 +62,65 @@ public static int compareRels(RelNode[] rels0, RelNode[]
rels1) {
return 0;
}
+ /** Returns whether a tree of {@link RelNode}s contains a match for a
+ * {@link RexNode} finder. */
+ public static boolean contains(RelNode rel,
+ Predicate<AggregateCall> aggPredicate, RexUtil.RexFinder finder) {
+ try {
+ findRex(rel, finder, aggPredicate, (relNode, rexNode) -> {
+ throw Util.FoundOne.NULL;
+ });
+ return false;
+ } catch (Util.FoundOne e) {
+ return true;
+ }
+ }
+
+ /** Searches for expressions in a tree of {@link RelNode}s. */
+ // TODO: a new method RelNode.accept(RexVisitor, BiConsumer), with similar
+ // overrides to RelNode.accept(RexShuttle), would be better.
+ public static void findRex(RelNode rel, RexUtil.RexFinder finder,
+ Predicate<AggregateCall> aggPredicate,
+ BiConsumer<RelNode, @Nullable RexNode> consumer) {
+ if (rel instanceof Filter) {
Review Comment:
this looks like it could be a visitor.
Maybe that's what the TODO above says?
##########
core/src/main/java/org/apache/calcite/sql/validate/MeasureScope.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License 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.apache.calcite.sql.validate;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlUtil;
+
+import com.google.common.collect.ImmutableList;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import static org.apache.calcite.util.Static.RESOURCE;
+
+import static java.util.stream.Collectors.joining;
+
+/**
+ * Scope for resolving identifiers within a SELECT item that is annotated
+ * "AS MEASURE".
+ *
+ * <p>Scope includes the identifiers of SELECT plus all aliases. This allows
+ * measures to reference each other and also reference other SELECT items.
+ */
+public class MeasureScope extends DelegatingScope {
+ //~ Instance fields --------------------------------------------------------
+
+ private final SqlSelect select;
+ private final List<String> aliasList;
+ private final Set<Integer> activeOrdinals = new HashSet<>();
+
+ /**
+ * Creates a MeasureScope.
+ *
+ * @param selectScope Parent scope
+ * @param select Enclosing SELECT node
+ */
+ MeasureScope(SqlValidatorScope selectScope,
+ SqlSelect select) {
+ super(selectScope);
+ this.select = select;
+
+ final List<String> aliasList = new ArrayList<>();
+ for (SqlNode selectItem : select.getSelectList()) {
+ aliasList.add(SqlValidatorUtil.alias(selectItem, aliasList.size()));
+ }
+ this.aliasList = ImmutableList.copyOf(aliasList);
+ }
+
+ @Override public SqlNode getNode() {
+ return select;
+ }
+
+ @Override public void validateExpr(SqlNode expr) {
+ SqlNode expanded = validator.extendedExpandGroupBy(expr, this, select);
+
+ // expression needs to be valid in parent scope too
+ parent.validateExpr(expanded);
+ }
+
+ @Override public @Nullable RelDataType resolveColumn(String name, SqlNode
ctx) {
+ final SqlNameMatcher matcher = validator.getCatalogReader().nameMatcher();
+ final int aliasOrdinal = matcher.indexOf(aliasList, name);
+ if (aliasOrdinal >= 0) {
+ final SqlNode selectItem = select.getSelectList().get(aliasOrdinal);
+ final SqlNode measure = SqlValidatorUtil.getMeasure(selectItem);
+ if (measure != null) {
+ try {
+ if (activeOrdinals.add(aliasOrdinal)) {
+ return validator.deriveType(this, measure);
+ }
+ final String dependentMeasures =
activeOrdinals.stream().map(aliasList::get)
+ .map(s -> "'" + s + "'")
+ .collect(joining(", "));
+ throw validator.newValidationError(ctx,
+ RESOURCE.measureIsCyclic(name, dependentMeasures));
+ } finally {
+ activeOrdinals.remove(aliasOrdinal);
+ }
+ }
+ final SqlNode expression = SqlUtil.stripAs(selectItem);
+ return validator.deriveType(parent, expression);
+ }
+ return super.resolveColumn(name, ctx);
+ }
+
+ public @Nullable SqlNode lookupMeasure(String name) {
+ final SqlNameMatcher matcher = validator.getCatalogReader().nameMatcher();
+ final int aliasOrdinal = matcher.indexOf(aliasList, name);
+ if (aliasOrdinal >= 0) {
+ final SqlNode selectItem = select.getSelectList().get(aliasOrdinal);
+ final @Nullable SqlNode measure =
SqlValidatorUtil.getMeasure(selectItem);
+ if (measure != null) {
+ return measure;
+ }
+ return SqlUtil.stripAs(selectItem); // non-measure select item
+ }
+ return null;
+ }
+
+ @Override public SqlQualified fullyQualify(SqlIdentifier identifier) {
+ // If it's a simple identifier, look for an alias.
+ if (identifier.isSimple()) {
+ @Nullable SqlQualified qualified = foo(this, select, identifier);
+ if (qualified != null) {
+ return qualified;
+ }
+ }
+ return super.fullyQualify(identifier);
+ }
+
+ static @Nullable SqlQualified foo(DelegatingScope scope, SqlSelect select,
Review Comment:
can this function have a better name?
##########
core/src/main/java/org/apache/calcite/sql/validate/OrderByScope.java:
##########
@@ -74,31 +74,43 @@ public class OrderByScope extends DelegatingScope {
// If it's a simple identifier, look for an alias.
if (identifier.isSimple()
&& validator.config().conformance().isSortByAlias()) {
- final String name = identifier.names.get(0);
- final SqlValidatorNamespace selectNs =
- validator.getNamespaceOrThrow(select);
- final RelDataType rowType = selectNs.getRowType();
-
- final SqlNameMatcher nameMatcher = validator.catalogReader.nameMatcher();
- final RelDataTypeField field = nameMatcher.field(rowType, name);
- final int aliasCount = aliasCount(nameMatcher, name);
- if (aliasCount > 1) {
- // More than one column has this alias.
- throw validator.newValidationError(identifier,
- RESOURCE.columnAmbiguous(name));
- }
- if (field != null && !field.isDynamicStar() && aliasCount == 1) {
- // if identifier is resolved to a dynamic star, use
super.fullyQualify() for such case.
- return SqlQualified.create(this, 1, selectNs, identifier);
+ SqlQualified qualified = foo(this, select, identifier);
+ if (qualified != null) {
+ return qualified;
}
}
return super.fullyQualify(identifier);
}
+ // This method was refactored out of fullyQualify so that it can be shared
+ // with MeasureScope;
+ // TODO: complete refactoring (perhaps moving to DelegatingScope);
+ // note that we have removed a check for DynamicStar.
+ static @Nullable SqlQualified foo(DelegatingScope scope, SqlSelect select,
Review Comment:
can this method have a better name?
##########
core/src/test/java/org/apache/calcite/test/RelBuilderTest.java:
##########
@@ -3279,6 +3280,126 @@ private static RelBuilder assertSize(RelBuilder b,
assertThat(root, hasTree(expected));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5802";>[CALCITE-5802]
+ * In RelBuilder, add method aggregateRex, to allow aggregating complex
+ * expressions such as "1 + SUM(x + 2)"</a>. */
+ @Test void testAggregateRex() {
+ // SELECT deptno,
+ // deptno + 2 AS d2,
+ // 3 + SUM(4 + sal) AS s
+ // FROM emp
+ // GROUP BY deptno
+ BiFunction<RelBuilder, Boolean, RelNode> f = (b, projectKey) ->
+ b.scan("EMP")
+ .aggregateRex(b.groupKey(b.field("DEPTNO")), projectKey,
+ ImmutableList.of(
+ b.alias(
+ b.call(SqlStdOperatorTable.PLUS, b.field("DEPTNO"),
+ b.literal(2)),
+ "d2"),
+ b.alias(
+ b.call(SqlStdOperatorTable.PLUS, b.literal(3),
+ b.call(SqlStdOperatorTable.SUM,
+ b.call(SqlStdOperatorTable.PLUS, b.literal(4),
+ b.field("SAL")))),
+ "s")))
+ .build();
+ final String expected = ""
+ + "LogicalProject(d2=[+($0, 2)], s=[+(3, $1)])\n"
+ + " LogicalAggregate(group=[{0}], agg#0=[SUM($1)])\n"
+ + " LogicalProject(DEPTNO=[$7], $f8=[+(4, $5)])\n"
+ + " LogicalTableScan(table=[[scott, EMP]])\n";
+ final String expectedRowType =
+ "RecordType(INTEGER d2, DECIMAL(19, 2) s) NOT NULL";
+ final RelNode r = f.apply(createBuilder(), false);
+ assertThat(r, hasTree(expected));
+ assertThat(r.getRowType().getFullTypeString(), is(expectedRowType));
+
+ // As above, with projectKey = true
+ final String expected2 = ""
+ + "LogicalProject(DEPTNO=[$0], d2=[+($0, 2)], s=[+(3, $1)])\n"
+ + " LogicalAggregate(group=[{0}], agg#0=[SUM($1)])\n"
+ + " LogicalProject(DEPTNO=[$7], $f8=[+(4, $5)])\n"
+ + " LogicalTableScan(table=[[scott, EMP]])\n";
+ final String expectedRowType2 =
+ "RecordType(TINYINT DEPTNO, INTEGER d2, DECIMAL(19, 2) s) NOT NULL";
+ final RelNode r2 = f.apply(createBuilder(), true);
+ assertThat(r2, hasTree(expected2));
+ assertThat(r2.getRowType().getFullTypeString(), is(expectedRowType2));
+ }
+
+ /** Tests {@link RelBuilder#aggregateRex} with an expression;
+ * it needs to be evaluated post aggregation. */
+ @Test void testAggregateRex2() {
+ // SELECT CURRENT_DATE AS d
+ // FROM emp
+ // GROUP BY ()
+ BiFunction<RelBuilder, Boolean, RelNode> f = (b, projectKey) ->
+ b.scan("EMP")
+ .aggregateRex(b.groupKey(), projectKey,
+ ImmutableList.of(
+ b.alias(b.call(SqlStdOperatorTable.CURRENT_DATE), "d")))
+ .build();
+ final String expected = ""
+ + "LogicalProject(d=[CURRENT_DATE])\n"
+ + " LogicalValues(tuples=[[{ true }]])\n";
+ final String expectedRowType = "RecordType(DATE NOT NULL d) NOT NULL";
+ final RelNode r = f.apply(createBuilder(), false);
+ assertThat(r, hasTree(expected));
+ assertThat(r.getRowType().getFullTypeString(), is(expectedRowType));
+
+ // As above, with projectKey = true
+ final RelNode r2 = f.apply(createBuilder(), true);
+ assertThat(r2, hasTree(expected));
+ assertThat(r2.getRowType().getFullTypeString(), is(expectedRowType));
+
+ // As above, disabling extra fields
+ final String expected3 = ""
+ + "LogicalProject(d=[CURRENT_DATE])\n"
+ + " LogicalValues(tuples=[[{ }]])\n";
+ final RelNode r3 =
+ f.apply(createBuilder(c -> c.withPreventEmptyFieldList(false)),
+ false);
+ assertThat(r3, hasTree(expected3));
+ assertThat(r3.getRowType().getFullTypeString(), is(expectedRowType));
+ }
+
+ /** Tests {@link RelBuilder#aggregateRex} with a literal expression;
+ * it needs to be evaluated post aggregation. */
+ @Test void testAggregateRex3() {
+ // SELECT 2 AS two, false AS f
+ // FROM emp
+ // GROUP BY ()
+ BiFunction<RelBuilder, Boolean, RelNode> f = (b, projectKey) ->
+ b.scan("EMP")
+ .aggregateRex(b.groupKey(), projectKey,
+ ImmutableList.of(b.alias(b.literal(2), "two"),
+ b.alias(b.literal(false), "f")))
+ .build();
+ final String expected =
+ "LogicalValues(tuples=[[{ 2, false }]])\n";
+ final String expectedRowType =
+ "RecordType(INTEGER NOT NULL two, BOOLEAN NOT NULL f) NOT NULL";
+ final RelNode r = f.apply(createBuilder(), false);
+ assertThat(r, hasTree(expected));
+ assertThat(r.getRowType().getFullTypeString(), is(expectedRowType));
+
+ // As above, with projectKey = true
+ final RelNode r2 = f.apply(createBuilder(), true);
+ assertThat(r2, hasTree(expected));
+ assertThat(r2.getRowType().getFullTypeString(), is(expectedRowType));
+
+ // As above, disabling extra fields
+ final String expected3 =
Review Comment:
this seems to be the same as expected
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidator.java:
##########
@@ -887,6 +889,25 @@ interface Config {
*/
Config withIdentifierExpansion(boolean expand);
+ /**
+ * Returns whether to treat the query being validated as top-level.
+ *
+ * <p>The default, false, treats the query as top-level;
+ * a value of true treats it as a query inside another, as would be the
case
+ * for a view or common table expression.
+ *
+ * <p>Possible behavior differences include ignoring the {@code ORDER BY}
+ * clause of an embedded query, or converting measure expressions of a
+ * non-embedded query into values. */
+ @Value.Default default boolean embedded() {
Review Comment:
Would "embeddedQuery" be a better name?
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -4001,6 +4001,222 @@ private void checkNegWindow(String s, String msg) {
.isAggregate(is(false));
}
+ @Test void testAsMeasure() {
+ // various kinds of measure expressions
+ sql("select deptno, empno + 1 as measure e1 from emp").ok();
+ sql("select *, empno + 1 as measure e1 from emp").ok();
+
+ // an aggregate function in a measure does not make it an aggregate query
+ sql("select *, sum(empno) as measure e1 from emp").ok();
+ sql("select e1 from (\n"
+ + " select deptno, empno + 1 as measure e1 from emp)").ok();
+ sql("select e1 from (\n"
+ + " select *, sum(empno) as measure e1 from emp)").ok();
+
+ // Aggregate and DISTINCT queries may not contain measures.
+ // (Maybe relax this restriction later?)
+ final String message = "MEASURE not valid in aggregate or DISTINCT query";
+ sql("select deptno, ^1 as measure e1^ from emp group by deptno")
+ .fails(message);
+ sql("select sum(sal) as s, ^1 as measure e1^ from emp having count(*) > 0")
+ .fails(message);
+ sql("select 2 + 3, count(*), ^1 as measure e1^ from emp")
+ .fails(message);
+ sql("select ^1 as measure e1^ from emp group by ()")
+ .fails(message);
+ sql("select distinct deptno, ^1 as measure e1^ from emp")
+ .fails(message);
+
+ // invalid column
+ sql("select\n"
+ + " ^nonExistent^ + 1 as d1,\n"
+ + " deptno + 3 as d3\n"
+ + "from emp").fails("Column 'NONEXISTENT' not found in any table");
+ sql("select\n"
+ + " ^nonExistent^ + 1 as measure d1,\n"
+ + " deptno + 3 as d3\n"
+ + "from emp").fails("Column 'NONEXISTENT' not found in any table");
+
+ // measures may reference both measures and non-measure aliases
+ sql("select\n"
+ + " deptno + 1 as d1,\n"
+ + " d1 + 2 as measure d3\n"
+ + "from emp").ok();
+ sql("select\n"
+ + " deptno + 1 as d1,\n"
+ + " d1 + 2 as measure d3,\n"
+ + " d3 + 3 as measure d6\n"
+ + "from emp").ok();
+ // forward references are ok
+ sql("select\n"
+ + " d3 + 3 as measure d6,\n"
+ + " d1 + 2 as measure d3,\n"
+ + " deptno + 1 as d1\n"
+ + "from emp").ok();
+ // non-measures may not reference measures
+ sql("select\n"
+ + " deptno + 1 as measure d1,\n"
+ + " ^d1^ + 2 as d3\n"
+ + "from emp").fails("Column 'D1' not found in any table");
+
+ // sub-query
+ sql("select * from (\n"
+ + " select deptno,\n"
+ + " empno + 1 as measure e1,\n"
+ + " e1 + deptno as measure e2\n"
+ + " from emp)").ok();
+ // as previous, but references a non-measure
+ sql("select * from (\n"
+ + " select deptno,\n"
+ + " empno + 1 as e1,\n"
+ + " e1 + deptno as measure e2\n"
+ + " from emp)").ok();
+
+ // non-measures don't even see measures - fall back to columns
+ final String intVarcharType =
+ "RecordType(INTEGER NOT NULL ENAME,"
+ + " VARCHAR(20) NOT NULL N) NOT NULL";
+ final String intVarcharmType =
Review Comment:
this seems to be identical. this may be clearer if the value of the previous
variable is used.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2381,6 +2384,53 @@ private static boolean canRollUp(TimeUnit outer,
TimeUnit inner) {
return false;
}
+ /** Simplifies a measure being converted immediately (in the same SELECT
+ * clause) back to a value.
+ *
+ * <p>For most expressions {@code e}, simplifies "{@code m2v(v2m(e))}" to
+ * "{@code e}". For example,
+ * "{@code SELECT deptno + 1 AS MEASURE m}"
+ * is equivalent to
+ * "{@code SELECT deptno + 1 AS m}".
+ *
+ * <p>The exception is aggregate functions.
+ * "{@code SELECT COUNT(*) + 1 AS MEASURE m}"
+ * simplifies to
+ * "{@code SELECT COUNT(*) OVER (ROWS CURRENT ROW) + 1 AS MEASURE m}".
+ *
+ * @param e Call to {@code M2V} to be simplified
+ * @return Simplified call
+ */
+ private RexNode simplifyM2v(RexCall e) {
+ assert e.op.kind == SqlKind.M2V;
+ final RexNode operand = e.getOperands().get(0);
+ switch (operand.getKind()) {
+ case V2M:
+ // M2V(V2M(x)) --> x
+ return flattenAggregate(((RexCall) operand).operands.get(0));
+ default:
+ return e;
Review Comment:
why doesn't this require a flattenAggregate call?
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java:
##########
@@ -1873,4 +1873,35 @@ public static boolean isValidDecimalValue(@Nullable
BigDecimal value, RelDataTyp
return true;
}
}
+
+ /** Strips MEASURE wrappers from a type.
+ *
+ * <p>For example:
+ * <ul>
+ * <li>"{@code INTEGER}" remains "{@code INTEGER}";
+ * <li>"{@code MEASURE<DECIMAL>}" becomes "{@code DECIMAL}";
+ * <li>"{@code (empno INTEGER NOT NULL, rating MEASURE<DECIMAL>)}"
+ * becomes "{@code (empno INTEGER NOT NULL, rating DECIMAL)}".
+ * </ul>
+ */
+ public static RelDataType fromMeasure(RelDataTypeFactory typeFactory,
+ RelDataType type) {
+ if (type.isStruct()) {
Review Comment:
what about other recursive types like array and map?
##########
core/src/test/java/org/apache/calcite/test/RelBuilderTest.java:
##########
@@ -3279,6 +3280,126 @@ private static RelBuilder assertSize(RelBuilder b,
assertThat(root, hasTree(expected));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5802";>[CALCITE-5802]
+ * In RelBuilder, add method aggregateRex, to allow aggregating complex
+ * expressions such as "1 + SUM(x + 2)"</a>. */
Review Comment:
does this PR include a fix to 5802?
In that case, could the fix be factored out as a separate PR?
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10189,11 +10405,6 @@ private static int prec(SqlOperator op) {
@Test void testDummy() {
// (To debug individual statements, paste them into this method.)
- expr("true\n"
Review Comment:
why delete these?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
