[GitHub] [calcite] xy2953396112 commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
xy2953396112 commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r516736309
##
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##
@@ -1931,6 +1931,18 @@ public static MutableRel
unifyAggregates(MutableAggregate query,
final List aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ int aggIndex = aggregateCall.getArgList().get(0);
Review comment:
Thanks, update the code.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on pull request #2239: Following [CALCITE-4332] correct the code style and add some description
chunweilei commented on pull request #2239: URL: https://github.com/apache/calcite/pull/2239#issuecomment-720270874 > isn't style mandatory in compiling phase , checking style plugin? No, some convention is not mandatory. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2229: Update release instructions
vlsi commented on a change in pull request #2229: URL: https://github.com/apache/calcite/pull/2229#discussion_r516059392 ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: > If they are not in sync, cherry-pick the missing commit(s) into `master`. This is exactly what `git checkout site; git rebase -i master` does, so it might be misleading to recommend `rebase -i master` and then suggest to cherry-pick. ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: It would be nice to publish documentation for all the versions (all released and the current development), then we could drop `site` branch altogether :) ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: You are right. Something like `git checkout master; git reset --hard site` is required to update master to the "new site". ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: AFAIK `git switch site; git rebase --empty=drop master; git switch master; git reset --hard site` should so the trick. Frankly speaking, the key problem for me was to understand the intention behind `site` vs `master` branch. The actual commands are not that hard once you understand why `site` and `master` can differ. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] danny0405 closed pull request #2218: [CALCITE-4233] Adding Elasticsearch Dismax API QueryBuilder
danny0405 closed pull request #2218: URL: https://github.com/apache/calcite/pull/2218 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] jcamachor commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
jcamachor commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r516184828
##
File path:
core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##
@@ -50,6 +50,12 @@
*/
public abstract class RelOptMaterializations {
+ @Deprecated // to be removed before 2.0
+ public static List>>
useMaterializedViews(
+ final RelNode rel, List materializations) {
+return useMaterializedViews(rel, materializations, null);
Review comment:
Should the third parameter be an empty list rather than null (I think
otherwise we may hit an NPE)?
##
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##
@@ -197,6 +198,10 @@ public boolean isRuleExcluded(RelOptRule rule) {
return this;
}
+ @Override public void addExtraMaterializationRules(List rules) {
Review comment:
Instead of having a mechanism to add _extra_ materialization rules, have
we thought about adding all materialization rules through this mechanism? That
way we can actually customize the full behavior rather than having a subset of
internally defined rules that are hardcoded.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2236: [CALCITE-3913] Test correctness using formal verification (Qi Zhou)
vlsi commented on pull request #2236: URL: https://github.com/apache/calcite/pull/2236#issuecomment-720787197 @qizhou92 , this looks nice. Have you seen `RexFuzzer`, `RexProgramFuzzyTest`, `org.apache.calcite.rex.RexSimplify#verify` ? I wonder if `z3` verifications could be integrated to `RexSimplify#verify`. Of course, it would be limited to `int` or something, but it still might be valuable. An alternative option is to add a dedicated case to `RexProgramFuzzyTest` which would generate `int-only` randomized expressions (e.g. via `RexFuzzer`), and verify if `RexSimplfy` does not corrupt them. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
chunweilei commented on pull request #2237: URL: https://github.com/apache/calcite/pull/2237#issuecomment-720301754 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2238: [CALCITE-4364] `a IN (1, 2) AND a = 1` should be simplified to `a = 1`
vlsi commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r516743019
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -1330,7 +1330,7 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
final SargCollector sargCollector = new SargCollector(rexBuilder, true);
operands.forEach(t -> sargCollector.accept(t, terms));
-if (sargCollector.map.values().stream().anyMatch(b -> b.complexity() > 1))
{
+if (sargCollector.needToFix(terms.size())) {
Review comment:
What do you mean `unnecessary plan change`?
Do you mean plan changes when compared with 1.25 or 1.26?
I guess 1.26 is not really viable (since there are significant issues with
Sarg), so I would skip 1.26 from consideration with regard to plan changes.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2236: [CALCITE-3913] Test correctness using formal verification (Qi Zhou)
vlsi commented on a change in pull request #2236:
URL: https://github.com/apache/calcite/pull/2236#discussion_r516285366
##
File path: core/src/test/java/org/apache/calcite/test/SmtLibTest.java
##
@@ -0,0 +1,115 @@
+/*
+ * 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.test;
+
+
+import org.apache.calcite.jdbc.JavaTypeFactoryImpl;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.logical.LogicalFilter;
+import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.test.verifier.RexToSymbolicColumn;
+import org.apache.calcite.test.verifier.SymbolicColumn;
+
+import com.microsoft.z3.BoolExpr;
+import com.microsoft.z3.Context;
+import com.microsoft.z3.Expr;
+import com.microsoft.z3.Solver;
+import com.microsoft.z3.Status;
+
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Testing for integrating z3 with calcite to verify equivalence.
+ **/
+
+public class SmtLibTest {
+
+ static RelDataTypeFactory typeFactory = new
JavaTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
+ /**
+ * Check z3 dynamic library exits before run all other tests.
+ */
+ @BeforeAll static void testZ3Lib() throws Exception {
+Context z3Context = new Context();
+Expr x = z3Context.mkIntConst("x");
+Expr one = z3Context.mkInt(1);
+BoolExpr eq = z3Context.mkEq(x, one);
+Solver solver = z3Context.mkSolver();
+solver.add(eq);
+assertEquals(solver.check(), Status.SATISFIABLE);
+ }
+
+ /** Converts a SQL string to a relational expression using mock schema. */
+ private static RelNode toRel(String sql) {
+final SqlToRelTestBase test = new SqlToRelTestBase() {
+};
+return test.createTester().convertSqlToRel(sql).rel;
+ }
+
+ /** Converts a where condition string to a RexNode using mock schema on emp
table. */
+ private static RexNode toRex(String cond) {
+final String sql = "select *\n"
++ "from emp where"
++ cond;
+final RelNode relNode = toRel(sql);
+LogicalProject project = (LogicalProject) relNode;
+LogicalFilter filter = (LogicalFilter) project.getInput();
+return filter.getCondition();
+ }
+
+ private static boolean checkEqual(String cond1, String cond2) {
+RexNode rexNode1 = toRex(cond1);
+RexNode rexNode2 = toRex(cond2);
+Context z3Context = new Context();
+List inputSymbolicColumns = new ArrayList<>();
+/** mock emp table, 8 columns with int type, since we only support
numerical type for now **/
+for (int i = 0; i < 8; i++) {
+ RelDataType intType = typeFactory.createJavaType(int.class);
+ SymbolicColumn inputColumn =
SymbolicColumn.mkNewSymbolicColumn(z3Context, intType);
+ inputSymbolicColumns.add(inputColumn);
Review comment:
Please use `RexProgramTestBase` so you could reuse all the helpers for
types, etc, etc.
##
File path: core/src/test/java/org/apache/calcite/test/SmtLibTest.java
##
@@ -0,0 +1,115 @@
+/*
+ * 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.test;
+
+
+import org.apache.calcite.jdbc.JavaTypeFactoryImpl;
+import
[GitHub] [calcite] chunweilei edited a comment on pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
chunweilei edited a comment on pull request #2237: URL: https://github.com/apache/calcite/pull/2237#issuecomment-720328998 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] chunweilei merged pull request #2239: Following [CALCITE-4332] correct the code style and add some description
chunweilei merged pull request #2239: URL: https://github.com/apache/calcite/pull/2239 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`
julianhyde commented on pull request #2238: URL: https://github.com/apache/calcite/pull/2238#issuecomment-720700806 Regarding the commit message: > `a in (1, 2) and a = 1` should be simplified to `a=1` Use upper-case for SQL, and spaces around `=`. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2163: [CALCITE-4273] Support get expression lineage for Calc
yanlin-Lynn commented on a change in pull request #2163:
URL: https://github.com/apache/calcite/pull/2163#discussion_r516402810
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
##
@@ -407,6 +409,35 @@ protected RelMdExpressionLineage() {}
return mq.getExpressionLineage(rel.getInput(), outputExpression);
}
+ /**
+ * Expression lineage from Calc.
+ */
+ public Set getExpressionLineage(Calc calc,
+ RelMetadataQuery mq, RexNode outputExpression) {
+final RelNode input = calc.getInput();
+final RexBuilder rexBuilder = calc.getCluster().getRexBuilder();
+// Extract input fields referenced by expression
+final ImmutableBitSet inputFieldsUsed = extractInputRefs(outputExpression);
+
+// Infer column origin expressions for given references
+final Map> mapping = new LinkedHashMap<>();
+Pair, ImmutableList> calcFilterAndProjects
=
+calc.getProgram().split();
Review comment:
OK, I'll update
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2229: Update release instructions
rubenada commented on pull request #2229: URL: https://github.com/apache/calcite/pull/2229#issuecomment-720524457 @julianhyde @danny0405 gentle reminder. Please take a look at the PR when you have some time. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] chunweilei merged pull request #2231: [CALCITE-4350] The reverse operation of collation direction is overly relaxed
chunweilei merged pull request #2231: URL: https://github.com/apache/calcite/pull/2231 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] qizhou92 commented on a change in pull request #2236: [CALCITE-3913] Test correctness using formal verification (Qi Zhou)
qizhou92 commented on a change in pull request #2236:
URL: https://github.com/apache/calcite/pull/2236#discussion_r516309298
##
File path: core/src/test/java/org/apache/calcite/test/SmtLibTest.java
##
@@ -0,0 +1,115 @@
+/*
+ * 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.test;
+
+
+import org.apache.calcite.jdbc.JavaTypeFactoryImpl;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.logical.LogicalFilter;
+import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.test.verifier.RexToSymbolicColumn;
+import org.apache.calcite.test.verifier.SymbolicColumn;
+
+import com.microsoft.z3.BoolExpr;
+import com.microsoft.z3.Context;
+import com.microsoft.z3.Expr;
+import com.microsoft.z3.Solver;
+import com.microsoft.z3.Status;
+
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Testing for integrating z3 with calcite to verify equivalence.
+ **/
+
+public class SmtLibTest {
+
+ static RelDataTypeFactory typeFactory = new
JavaTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
+ /**
+ * Check z3 dynamic library exits before run all other tests.
+ */
+ @BeforeAll static void testZ3Lib() throws Exception {
+Context z3Context = new Context();
+Expr x = z3Context.mkIntConst("x");
+Expr one = z3Context.mkInt(1);
+BoolExpr eq = z3Context.mkEq(x, one);
+Solver solver = z3Context.mkSolver();
+solver.add(eq);
+assertEquals(solver.check(), Status.SATISFIABLE);
+ }
+
+ /** Converts a SQL string to a relational expression using mock schema. */
+ private static RelNode toRel(String sql) {
+final SqlToRelTestBase test = new SqlToRelTestBase() {
+};
+return test.createTester().convertSqlToRel(sql).rel;
+ }
+
+ /** Converts a where condition string to a RexNode using mock schema on emp
table. */
+ private static RexNode toRex(String cond) {
+final String sql = "select *\n"
++ "from emp where"
++ cond;
+final RelNode relNode = toRel(sql);
+LogicalProject project = (LogicalProject) relNode;
+LogicalFilter filter = (LogicalFilter) project.getInput();
+return filter.getCondition();
+ }
+
+ private static boolean checkEqual(String cond1, String cond2) {
+RexNode rexNode1 = toRex(cond1);
+RexNode rexNode2 = toRex(cond2);
+Context z3Context = new Context();
+List inputSymbolicColumns = new ArrayList<>();
+/** mock emp table, 8 columns with int type, since we only support
numerical type for now **/
+for (int i = 0; i < 8; i++) {
+ RelDataType intType = typeFactory.createJavaType(int.class);
+ SymbolicColumn inputColumn =
SymbolicColumn.mkNewSymbolicColumn(z3Context, intType);
+ inputSymbolicColumns.add(inputColumn);
Review comment:
Hi, thank you for your comments. I use **RexProgramBuilderBase**, which
solves the problem. And **RexProgramTestBase** is not a public class.
##
File path: core/src/test/java/org/apache/calcite/test/SmtLibTest.java
##
@@ -0,0 +1,115 @@
+/*
+ * 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.test;
+
+
[GitHub] [calcite] qizhou92 commented on pull request #2236: [CALCITE-3913] Test correctness using formal verification (Qi Zhou)
qizhou92 commented on pull request #2236: URL: https://github.com/apache/calcite/pull/2236#issuecomment-720752477 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] eolivelli commented on a change in pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`
eolivelli commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r516232865
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -2710,6 +2710,16 @@ private boolean accept1(RexNode e, SqlKind kind,
}
}
+/** Checks whether it is worth to fix and convert to {@code SEARCH} calls.
*/
+boolean needToFix(int newTermsCnt) {
+ // Fix and converts to SEARCH if:
+ // 1. A Sarg has complexity greater than 1;
+ // 2. The terms are reduced as simpler Sarg points.
+ return map.values().stream().anyMatch(b -> b.complexity() > 1)
Review comment:
Streams are cool but they are very expensive.
Probably we are in some hot path here.
Is it worth to use a simple loop?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
vlsi commented on pull request #2237: URL: https://github.com/apache/calcite/pull/2237#issuecomment-720322176 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] xy2953396112 opened a new pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
xy2953396112 opened a new pull request #2241: URL: https://github.com/apache/calcite/pull/2241 …nct aggregate on target GROUP BY column (xzh) https://issues.apache.org/jira/browse/CALCITE-4374 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2229: Update release instructions
rubenada commented on a change in pull request #2229: URL: https://github.com/apache/calcite/pull/2229#discussion_r516067535 ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: You're right, I will re-phrase it. ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: Agree, there was a discussion about that some time ago [1], but I think no action has been taken in that direction. [1] https://lists.apache.org/thread.html/0d9829f7e32f51bc03fc350fe7c782c03dedb2ecca90e983917abf53%40%3Cdev.calcite.apache.org%3E ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: @vlsi correct me if I'm wrong. The goal of this point is to ensure that "there is no commit on `site` that has not been applied also to `master`", otherwise we should apply these commits (onto `master`). Doing a `git checkout site; git rebase -i master` will point the commits on `site` that are not in `master` (if any); but will not modify the `master` branch. That is why the `cherry-pick` tried to express; or am I wrong? ## File path: site/_docs/howto.md ## @@ -684,8 +688,13 @@ Note: release artifacts (dist.apache.org and repository.apache.org) are managed Before you start: +* Send an email to [d...@calcite.apache.org](mailto:d...@calcite.apache.org) notifying that RC build process + is starting and therefore `master` branch is in code freeze until further notice. * Set up signing keys as described above. * Make sure you are using JDK 8 (not 9 or 10). +* Make sure `master` branch and `site` branch are in sync, i.e. there is no commit on `site` that has not + been applied also to `master`. You can check by doing `git checkout site; git rebase -i master`. + If they are not in sync, cherry-pick the missing commit(s) into `master`. Review comment: We need to be careful because there can be site-related commits in `master` that are not in `site`, this is "allowed" and we must not lose them. Having commits in `site` that are not in `master` should not happen, but it can be possible if someone makes a (small) mistake. All in all, I think the current phrase (`git checkout site; git rebase -i master` + `cherry-pick` any missing commits into master) might be the safest approach to achieve this without losing any master commit. wdyt? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
yanlin-Lynn commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r516694228
##
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##
@@ -1931,6 +1931,18 @@ public static MutableRel
unifyAggregates(MutableAggregate query,
final List aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ int aggIndex = aggregateCall.getArgList().get(0);
Review comment:
What about other args ?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asfgit closed pull request #2225: [CALCITE-4345] NPE in AggregateCaseToFilterRule when converts SUM(CASE WHEN c THEN 1 END) (Jiatao Tao)
asfgit closed pull request #2225: URL: https://github.com/apache/calcite/pull/2225 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
xy2953396112 commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r516391375
##
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##
@@ -197,6 +198,10 @@ public boolean isRuleExcluded(RelOptRule rule) {
return this;
}
+ @Override public void addExtraMaterializationRules(List rules) {
Review comment:
The materialized recognition rules currently implemented are quite
general (based on pure relational algebra). I think adding some additional
rules for the materialized recognition of specific scenes will help us to solve
the materialized recognition of specific scenes, rather than adding them all
together. Even if no additional rules are added, the current physical and
chemical recognition rules, such as `CalcToCalcUnifyRule` and
`JoinOnCalcsToJoinUnifyRule`, are still the most basic materialized recognition
rules, and we should keep them unchanged.I think it's better to retain the most
basic materialized recognition rules and add custom materialized recognition
rules at the same time.
##
File path:
core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
##
@@ -50,6 +50,12 @@
*/
public abstract class RelOptMaterializations {
+ @Deprecated // to be removed before 2.0
+ public static List>>
useMaterializedViews(
+ final RelNode rel, List materializations) {
+return useMaterializedViews(rel, materializations, null);
Review comment:
Thanks, update the code.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2223: Proposal : 4344 test container sample
zabetak commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r515817104
##
File path:
core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##
@@ -239,6 +239,12 @@
public static final CalciteSystemProperty TEST_REDIS =
booleanProperty("calcite.test.redis", true);
+ /**
+ * Whether to use Docker with TestContainers
(https://www.testcontainers.org/) when available
+ */
+ public static final CalciteSystemProperty
TEST_WITH_DOCKER_CONTAINER =
+ booleanProperty("calcite.test.docker", true);
+
Review comment:
What's the benefit of having this property?
If Docker is available we can run the tests, if not then either we skip them
or fallback to another execution method.
##
File path:
redis/src/test/java/org/apache/calcite/adapter/redis/RedisCaseBase.java
##
@@ -36,16 +43,33 @@
public static final String HOST = "127.0.0.1";
private static final String MAX_HEAP = "maxheap 5120";
+ // If Docker is running, a container will be used
+ public static final GenericContainer REDIS_CONTAINER =
+ new GenericContainer<>("redis").withExposedPorts(6379);
Review comment:
I agree, let's freeze the version ideally using the same that is present
in the gradle.properties.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
chunweilei commented on a change in pull request #2237:
URL: https://github.com/apache/calcite/pull/2237#discussion_r515812551
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -311,31 +312,39 @@ default boolean onProduce(RelNode node) {
}
if (group.taskState != null && upperBound.isLe(group.upperBound)) {
-// either this group failed to optimize before or it is a ring
+// Either this group failed to optimize before or it is a ring.
return;
}
group.startOptimize(upperBound);
- // cannot decide an actual lower bound before MExpr are fully explored
- // so delay the lower bound checking
+ // Cannot decide an actual lower bound before MExpr are fully explored.
+ // So delay the lower bound checking.
- // a gate keeper to update context
+ // A gate keeper to update context.
tasks.push(new GroupOptimized(group));
- // optimize mExprs in group
+ // Optimize mExprs in group.
List physicals = new ArrayList<>();
for (RelNode rel : group.set.rels) {
if (planner.isLogical(rel)) {
tasks.push(new OptimizeMExpr(rel, group, false));
} else if (rel.isEnforcer()) {
- // Enforcers have lower priority than other physical nodes
+ // Enforcers have lower priority than other physical nodes.
physicals.add(0, rel);
} else {
physicals.add(rel);
}
}
- // always apply O_INPUTS first so as to get an valid upper bound
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
+ // It only happens when enabling AbstractConverter.
+ if (group.getConvention() == Convention.NONE) {
+LOGGER.debug("Skip optimizing because of none convention: {}", group);
+return;
+ }
Review comment:
@vlsi Here is the main change.
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
It means OptimzeInput, which is a term in Cascades planner. I borrow it
from the comment[1].
[1]
https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java#L338
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
I would change it as you suggest. But the term O_INPUT is not invented
currently. It comes from a classic paper[1] which was published in 1998.
[1]
https://15721.courses.cs.cmu.edu/spring2019/papers/22-optimizer1/xu-columbia-thesis1998.pdf
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
I would change it as you suggest. But the term O_INPUT is not invented
currently. It comes from a classic paper[1] which was published in 1998.
[1]
https://15721.courses.cs.cmu.edu/spring2019/papers/22-optimizer1/xu-columbia-thesis1998.pdf
Page#63
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde opened a new pull request #2240: Work-in-progress; do not review; do not merge
julianhyde opened a new pull request #2240: URL: https://github.com/apache/calcite/pull/2240 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2223: Proposal : 4344 test container sample
vlsi commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r515826195
##
File path:
core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##
@@ -239,6 +239,12 @@
public static final CalciteSystemProperty TEST_REDIS =
booleanProperty("calcite.test.redis", true);
+ /**
+ * Whether to use Docker with TestContainers
(https://www.testcontainers.org/) when available
+ */
+ public static final CalciteSystemProperty
TEST_WITH_DOCKER_CONTAINER =
+ booleanProperty("calcite.test.docker", true);
+
Review comment:
@tgrall , would you please add a corresponding comment to the javadoc?
I believe the key point of having such a property is to allow running a
non-docker test even in the case Docker is installed on the developer's machine.
For instance, it might be helpful to replicate an issue that happens only in
non-docker test mode.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] zinking commented on pull request #2239: Following [CALCITE-4332] correct the code style and add some description
zinking commented on pull request #2239: URL: https://github.com/apache/calcite/pull/2239#issuecomment-720242103 isn't style mandatory in compiling phase , checking style plugin? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`
danny0405 commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r516407768
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -1330,7 +1330,7 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
final SargCollector sargCollector = new SargCollector(rexBuilder, true);
operands.forEach(t -> sargCollector.accept(t, terms));
-if (sargCollector.map.values().stream().anyMatch(b -> b.complexity() > 1))
{
+if (sargCollector.needToFix(terms.size())) {
Review comment:
The `drawback` is there are many unnecessary plan change.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`
julianhyde commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r516228159
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -2710,6 +2710,16 @@ private boolean accept1(RexNode e, SqlKind kind,
}
}
+/** Checks whether it is worth to fix and convert to {@code SEARCH} calls.
*/
Review comment:
s/Checks/Returns/; "Checks" often means that the method will throw if
the check fails, whereas "Returns" is unambiguous.
Put the criteria in the javadoc, and describe the purpose of `newTermsCnt`.
I wouldn't abbreviate `count` to `cnt`. Save to characters, but convert a
word into a non-word.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
vlsi commented on a change in pull request #2237:
URL: https://github.com/apache/calcite/pull/2237#discussion_r515870606
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
What is `O_INPUTS`?
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
I see no reasons to invent obscure words when the thing can be explained
in plain English.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] danny0405 commented on pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
danny0405 commented on pull request #2237: URL: https://github.com/apache/calcite/pull/2237#issuecomment-720289937 Remember to update the commit message before commit. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2163: [CALCITE-4273] Support get expression lineage for Calc
danny0405 commented on a change in pull request #2163:
URL: https://github.com/apache/calcite/pull/2163#discussion_r515829778
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
##
@@ -407,6 +409,35 @@ protected RelMdExpressionLineage() {}
return mq.getExpressionLineage(rel.getInput(), outputExpression);
}
+ /**
+ * Expression lineage from Calc.
+ */
+ public Set getExpressionLineage(Calc calc,
+ RelMetadataQuery mq, RexNode outputExpression) {
+final RelNode input = calc.getInput();
+final RexBuilder rexBuilder = calc.getCluster().getRexBuilder();
+// Extract input fields referenced by expression
+final ImmutableBitSet inputFieldsUsed = extractInputRefs(outputExpression);
+
+// Infer column origin expressions for given references
+final Map> mapping = new LinkedHashMap<>();
+Pair, ImmutableList> calcFilterAndProjects
=
+calc.getProgram().split();
Review comment:
`calcFilterAndProjects ` -> `calcProjectsAndFilter` ? Because projects
are the key part.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`
vlsi commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r516246682
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -1330,7 +1330,7 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
final SargCollector sargCollector = new SargCollector(rexBuilder, true);
operands.forEach(t -> sargCollector.accept(t, terms));
-if (sargCollector.map.values().stream().anyMatch(b -> b.complexity() > 1))
{
+if (sargCollector.needToFix(terms.size())) {
Review comment:
What if we always convert to sarg format? Are there drawbacks?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`
liyafan82 commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r516407877
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -2710,6 +2710,16 @@ private boolean accept1(RexNode e, SqlKind kind,
}
}
+/** Checks whether it is worth to fix and convert to {@code SEARCH} calls.
*/
+boolean needToFix(int newTermsCnt) {
+ // Fix and converts to SEARCH if:
+ // 1. A Sarg has complexity greater than 1;
+ // 2. The terms are reduced as simpler Sarg points.
+ return map.values().stream().anyMatch(b -> b.complexity() > 1)
Review comment:
+ 1 for this suggestion.
By using a simple loop, we can combine the two stream expressions?
In addition, checking condition `newTermsCnt == 1` is cheap, can we move it
forward?
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -2710,6 +2710,16 @@ private boolean accept1(RexNode e, SqlKind kind,
}
}
+/** Checks whether it is worth to fix and convert to {@code SEARCH} calls.
*/
+boolean needToFix(int newTermsCnt) {
+ // Fix and converts to SEARCH if:
+ // 1. A Sarg has complexity greater than 1;
+ // 2. The terms are reduced as simpler Sarg points.
+ return map.values().stream().anyMatch(b -> b.complexity() > 1)
Review comment:
+1 for this suggestion.
By using a simple loop, we can combine the two stream expressions?
In addition, checking condition `newTermsCnt == 1` is cheap, can we move it
forward?
##
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##
@@ -1774,6 +1770,17 @@ private void checkExponentialCnf(int n) {
checkSimplify2(e, "SEARCH(?0.int0, Sarg[10])", "=(?0.int0, 10)");
}
+ @Test void testSimplifyInAnd() {
Review comment:
Maybe we need another test case:
deptno in (20, 10) and deptno = 30
==> false?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] danny0405 closed pull request #2214: [CALCITE-4106] Consider "listCoerced" in TypeCoercionImpl#inOperationCoercion (Jiatao Tao)
danny0405 closed pull request #2214: URL: https://github.com/apache/calcite/pull/2214 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] danny0405 commented on pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`
danny0405 commented on pull request #2238: URL: https://github.com/apache/calcite/pull/2238#issuecomment-720289249 > We should merge two sargs if they apply to the same argument Totally agree, and actually this patch makes more sargs merged. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] jcamachor commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
jcamachor commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r516873161
##
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##
@@ -197,6 +198,10 @@ public boolean isRuleExcluded(RelOptRule rule) {
return this;
}
+ @Override public void addExtraMaterializationRules(List rules) {
Review comment:
Yes, I understand that they are quite general. My point was that since
we are already changing the interface, we could use the same method to add any
materialization rules, allowing full control over which ones are enabled
throughout the planning. However, I am fine with this approach if others think
it is fine.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[calcite] branch master updated: [CALCITE-4273] Support get expression lineage for Calc
This is an automated email from the ASF dual-hosted git repository.
yanlin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new 2e9edae [CALCITE-4273] Support get expression lineage for Calc
2e9edae is described below
commit 2e9edae7fc57ab9c9c7c097008724ac99a1791a3
Author: yanlin-Lynn <1989yanlinw...@163.com>
AuthorDate: Tue Sep 22 18:02:11 2020 +0800
[CALCITE-4273] Support get expression lineage for Calc
---
.../rel/metadata/RelMdExpressionLineage.java | 31 ++
.../org/apache/calcite/test/RelMetadataTest.java | 21 +++
2 files changed, 52 insertions(+)
diff --git
a/core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
b/core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
index 41189a0..f1c97cb 100644
---
a/core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
+++
b/core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
@@ -21,6 +21,7 @@ import org.apache.calcite.plan.hep.HepRelVertex;
import org.apache.calcite.plan.volcano.RelSubset;
import org.apache.calcite.rel.RelNode;
import org.apache.calcite.rel.core.Aggregate;
+import org.apache.calcite.rel.core.Calc;
import org.apache.calcite.rel.core.Exchange;
import org.apache.calcite.rel.core.Filter;
import org.apache.calcite.rel.core.Join;
@@ -42,6 +43,7 @@ import org.apache.calcite.rex.RexUtil;
import org.apache.calcite.sql.validate.SqlValidatorUtil;
import org.apache.calcite.util.BuiltInMethod;
import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.Pair;
import org.apache.calcite.util.Util;
import com.google.common.collect.HashMultimap;
@@ -407,6 +409,35 @@ public class RelMdExpressionLineage
}
/**
+ * Expression lineage from Calc.
+ */
+ public Set getExpressionLineage(Calc calc,
+ RelMetadataQuery mq, RexNode outputExpression) {
+final RelNode input = calc.getInput();
+final RexBuilder rexBuilder = calc.getCluster().getRexBuilder();
+// Extract input fields referenced by expression
+final ImmutableBitSet inputFieldsUsed = extractInputRefs(outputExpression);
+
+// Infer column origin expressions for given references
+final Map> mapping = new LinkedHashMap<>();
+Pair, ImmutableList> calcProjectsAndFilter
=
+calc.getProgram().split();
+for (int idx : inputFieldsUsed) {
+ final RexNode inputExpr = calcProjectsAndFilter.getKey().get(idx);
+ final Set originalExprs = mq.getExpressionLineage(input,
inputExpr);
+ if (originalExprs == null) {
+// Bail out
+return null;
+ }
+ final RexInputRef ref = RexInputRef.of(idx,
calc.getRowType().getFieldList());
+ mapping.put(ref, originalExprs);
+}
+
+// Return result
+return createAllPossibleExpressions(rexBuilder, outputExpression, mapping);
+ }
+
+ /**
* Given an expression, it will create all equivalent expressions resulting
* from replacing all possible combinations of references in the mapping by
* the corresponding expressions.
diff --git a/core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
b/core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
index bfda204..e181a0b 100644
--- a/core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
+++ b/core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
@@ -2507,6 +2507,27 @@ public class RelMetadataTest extends SqlToRelTestBase {
assertNull(r);
}
+ @Test void testExpressionLineageCalc() {
+final RelNode rel = convertSql("select sal from (\n"
++ " select deptno, empno, sal + 1 as sal, job from emp) "
++ "where deptno = 10");
+final HepProgramBuilder programBuilder = HepProgram.builder();
+programBuilder.addRuleInstance(CoreRules.PROJECT_TO_CALC);
+programBuilder.addRuleInstance(CoreRules.FILTER_TO_CALC);
+programBuilder.addRuleInstance(CoreRules.CALC_MERGE);
+final HepPlanner planner = new HepPlanner(programBuilder.build());
+planner.setRoot(rel);
+final RelNode optimizedRel = planner.findBestExp();
+final RelMetadataQuery mq = optimizedRel.getCluster().getMetadataQuery();
+
+final RexNode ref = RexInputRef.of(0,
optimizedRel.getRowType().getFieldList());
+final Set r = mq.getExpressionLineage(optimizedRel, ref);
+
+assertThat(r.size(), is(1));
+final String resultString = r.iterator().next().toString();
+assertThat(resultString, is("+([CATALOG, SALES, EMP].#0.$5, 1)"));
+ }
+
@Test void testAllPredicates() {
final Project rel = (Project) convertSql("select * from emp, dept");
final Join join = (Join) rel.getInput();
[GitHub] [calcite] yanlin-Lynn merged pull request #2163: [CALCITE-4273] Support get expression lineage for Calc
yanlin-Lynn merged pull request #2163: URL: https://github.com/apache/calcite/pull/2163 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2238: [CALCITE-4364] `a IN (1, 2) AND a = 1` should be simplified to `a = 1`
danny0405 commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r517056551
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -1330,7 +1330,7 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
final SargCollector sargCollector = new SargCollector(rexBuilder, true);
operands.forEach(t -> sargCollector.accept(t, terms));
-if (sargCollector.map.values().stream().anyMatch(b -> b.complexity() > 1))
{
+if (sargCollector.needToFix(terms.size())) {
Review comment:
For example, a simple `$0=1` would be converted to `Sarg` which is
meaningless.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] liyafan82 opened a new pull request #2242: [CALCITE-4363] Need a utility to check if a SQL operator is standard
liyafan82 opened a new pull request #2242: URL: https://github.com/apache/calcite/pull/2242 Please see https://issues.apache.org/jira/browse/CALCITE-4363 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517082361
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -311,31 +311,31 @@ default boolean onProduce(RelNode node) {
}
if (group.taskState != null && upperBound.isLe(group.upperBound)) {
-// either this group failed to optimize before or it is a ring
+// Either this group failed to optimize before or it is a ring.
return;
}
group.startOptimize(upperBound);
- // cannot decide an actual lower bound before MExpr are fully explored
- // so delay the lower bound checking
+ // Cannot decide an actual lower bound before MExpr are fully explored.
+ // So delay the lower bound check.
- // a gate keeper to update context
+ // A gate keeper to update context.
tasks.push(new GroupOptimized(group));
- // optimize mExprs in group
+ // Optimize mExprs in group.
List physicals = new ArrayList<>();
for (RelNode rel : group.set.rels) {
if (planner.isLogical(rel)) {
tasks.push(new OptimizeMExpr(rel, group, false));
} else if (rel.isEnforcer()) {
- // Enforcers have lower priority than other physical nodes
+ // Enforcers have lower priority than other physical nodes.
physicals.add(0, rel);
} else {
physicals.add(rel);
}
}
- // always apply O_INPUTS first so as to get an valid upper bound
+ // Always apply O_INPUTS first so as to get an valid upper bound.
Review comment:
an -> a
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517083852
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -768,22 +772,22 @@ private boolean checkLowerBound(RelNode rel, RelSubset
group) {
@Override public void perform() {
if (input != parent.getInput(i)) {
-// The input has chnaged. So reschedule the optimize task.
+// The input has changed. So reschedule the optimize task.
input = (RelSubset) parent.getInput(i);
tasks.push(this);
tasks.push(new OptimizeGroup(input, upper));
return;
}
- // Optimize input completed. Update the context for other inputs
+ // Optimize input completed. Update the context for other inputs.
Review comment:
Optimize -> Optimizing
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
chunweilei commented on pull request #2243: URL: https://github.com/apache/calcite/pull/2243#issuecomment-721499337 @liyafan82 thank you for your review. All comments are addressed. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2242: [CALCITE-4363] Need a utility to check if a SQL operator is standard
liyafan82 commented on a change in pull request #2242:
URL: https://github.com/apache/calcite/pull/2242#discussion_r517087047
##
File path:
core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java
##
@@ -482,6 +500,7 @@
* Its precedence is less than the prefix {@link #UNARY_PLUS +}
* and {@link #UNARY_MINUS -} operators.
*/
+ @LibraryOperator(libraries = {STANDARD})
public static final SqlBinaryOperator MINUS =
new SqlMonotonicBinaryOperator(
"-",
Review comment:
Right, as @julianhyde pointed out, some operators are only for internal
use
(https://issues.apache.org/jira/browse/CALCITE-4363?focusedCommentId=17223122=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17223122).
In addition, some operators are supported by most databases, but not all
databases (e.g. sqrt).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] chunweilei opened a new pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
chunweilei opened a new pull request #2243: URL: https://github.com/apache/calcite/pull/2243 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on pull request #2242: [CALCITE-4363] Need a utility to check if a SQL operator is standard
liyafan82 commented on pull request #2242: URL: https://github.com/apache/calcite/pull/2242#issuecomment-721482425 This PR does two things: 1. We provide a utility to get the related sql libraries, given a sql operator. This should be helpful for the users (see the discussion in the JIRA) 2. We annotate some sql operators as 'standard'. In general, the standard sql operators are from two sources: 1) internal arithmetic operations (e.g. +, -, *, /); 2) ANSI SQL functions (see https://docs.oracle.com/cd/E57185_01/IRWUG/ch12s04s05.html). So it would be safe to annotate them as 'standard'. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
chunweilei commented on pull request #2237: URL: https://github.com/apache/calcite/pull/2237#issuecomment-721482956 Opened another PR to fix comments: https://github.com/apache/calcite/pull/2243. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2242: [CALCITE-4363] Need a utility to check if a SQL operator is standard
chunweilei commented on a change in pull request #2242:
URL: https://github.com/apache/calcite/pull/2242#discussion_r517072732
##
File path:
core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java
##
@@ -482,6 +500,7 @@
* Its precedence is less than the prefix {@link #UNARY_PLUS +}
* and {@link #UNARY_MINUS -} operators.
*/
+ @LibraryOperator(libraries = {STANDARD})
public static final SqlBinaryOperator MINUS =
new SqlMonotonicBinaryOperator(
"-",
Review comment:
So does it mean that not all operators in SqlStdOperatirTable belong to
STANDARD?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
liyafan82 commented on a change in pull request #2243: URL: https://github.com/apache/calcite/pull/2243#discussion_r517080225 ## File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java ## @@ -69,8 +69,8 @@ private GeneratorTask applying = null; /** - * RelNodes that are generated by passThrough or derive - * these nodes will not takes part in another passThrough or derive. + * RelNodes that are generated by passThrough or derive. + * These nodes will not take part in another passThrough or derive. Review comment: It would be helpful to add some links to the `passThrough` and `derive` operations. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517081499
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -170,27 +170,27 @@ private void clearProcessed(RelSet set) {
}
}
- // a callback invoked when a RelNode is going to be added into a RelSubset,
- // either by Register or Reregister. The task driver should need to schedule
- // tasks for the new nodes.
+ // A callback invoked when a RelNode is going to be added into a RelSubset,
+ // either by Register or Reregister. The task driver should schedule tasks
+ // for the new nodes.
@Override public void onProduce(RelNode node, RelSubset subset) {
-// if the RelNode is added to another RelSubset, just ignore it.
-// It should be schedule in the later OptimizeGroup task
+// If the RelNode is added to another RelSubset, just ignore it.
+// It should be scheduled in the later OptimizeGroup task.
if (applying == null || subset.set
!= VolcanoPlanner.equivRoot(applying.group().set)) {
return;
}
-// extra callback from each task
+// Extra callback from each task.
if (!applying.onProduce(node)) {
return;
}
if (!planner.isLogical(node)) {
// For a physical node, schedule tasks to optimize its inputs.
// The upper bound depends on all optimizing RelSubsets that this Rel
belongs to.
Review comment:
Rel -> RelNode?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517083461
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -601,7 +604,7 @@ private boolean checkLowerBound(RelNode rel, RelSubset
group) {
}
/**
- * A task that optimize input for physical nodes who has only one input.
+ * A task that optimizes input for physical nodes who has only one input.
* This task can be replaced by OptimizeInputs but simplify lots of logic.
Review comment:
simplify -> simplifies
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517083286
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -590,7 +593,7 @@ private RelNode convert(RelNode rel, RelSubset group) {
return null;
}
- // check whether a node's lower bound is less than a RelSubset's upper bound
+ // Check whether a node's lower bound is less than a RelSubset's upper bound.
Review comment:
Change this to a JavaDoc, and "Check" -> "Checks"?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
chunweilei commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517085070
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -311,31 +311,31 @@ default boolean onProduce(RelNode node) {
}
if (group.taskState != null && upperBound.isLe(group.upperBound)) {
-// either this group failed to optimize before or it is a ring
+// Either this group failed to optimize before or it is a ring.
return;
}
group.startOptimize(upperBound);
- // cannot decide an actual lower bound before MExpr are fully explored
- // so delay the lower bound checking
+ // Cannot decide an actual lower bound before MExpr are fully explored.
+ // So delay the lower bound check.
- // a gate keeper to update context
+ // A gate keeper to update context.
tasks.push(new GroupOptimized(group));
- // optimize mExprs in group
+ // Optimize mExprs in group.
List physicals = new ArrayList<>();
for (RelNode rel : group.set.rels) {
if (planner.isLogical(rel)) {
tasks.push(new OptimizeMExpr(rel, group, false));
} else if (rel.isEnforcer()) {
- // Enforcers have lower priority than other physical nodes
+ // Enforcers have lower priority than other physical nodes.
physicals.add(0, rel);
} else {
physicals.add(rel);
}
}
- // always apply O_INPUTS first so as to get an valid upper bound
+ // Always apply O_INPUTS first so as to get an valid upper bound.
Review comment:
Done.
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##
@@ -170,27 +170,27 @@ private void clearProcessed(RelSet set) {
}
}
- // a callback invoked when a RelNode is going to be added into a RelSubset,
- // either by Register or Reregister. The task driver should need to schedule
- // tasks for the new nodes.
+ // A callback invoked when a RelNode is going to be added into a RelSubset,
+ // either by Register or Reregister. The task driver should schedule tasks
+ // for the new nodes.
@Override public void onProduce(RelNode node, RelSubset subset) {
-// if the RelNode is added to another RelSubset, just ignore it.
-// It should be schedule in the later OptimizeGroup task
+// If the RelNode is added to another RelSubset, just ignore it.
+// It should be scheduled in the later OptimizeGroup task.
if (applying == null || subset.set
!= VolcanoPlanner.equivRoot(applying.group().set)) {
return;
}
-// extra callback from each task
+// Extra callback from each task.
if (!applying.onProduce(node)) {
return;
}
if (!planner.isLogical(node)) {
// For a physical node, schedule tasks to optimize its inputs.
// The upper bound depends on all optimizing RelSubsets that this Rel
belongs to.
Review comment:
Done.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
chunweilei commented on a change in pull request #2243: URL: https://github.com/apache/calcite/pull/2243#discussion_r517085147 ## File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java ## @@ -69,8 +69,8 @@ private GeneratorTask applying = null; /** - * RelNodes that are generated by passThrough or derive - * these nodes will not takes part in another passThrough or derive. + * RelNodes that are generated by passThrough or derive. + * These nodes will not take part in another passThrough or derive. Review comment: Sounds reasonable. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
chunweilei commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r517086483
##
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##
@@ -1931,6 +1931,22 @@ public static MutableRel
unifyAggregates(MutableAggregate query,
final List aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ if (aggregateCall.getArgList().size() == 1) {
+int aggIndex = aggregateCall.getArgList().get(0);
+if (target.groupSet.asSet().contains(aggIndex)) {
Review comment:
Could you please clarify why the size of the arguments has to be 1?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
xy2953396112 commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r517118764
##
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##
@@ -1931,6 +1931,22 @@ public static MutableRel
unifyAggregates(MutableAggregate query,
final List aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ if (aggregateCall.getArgList().size() == 1) {
+int aggIndex = aggregateCall.getArgList().get(0);
+if (target.groupSet.asSet().contains(aggIndex)) {
Review comment:
The aggregate function containing the distinct keyword has only one
parameter. Check the parameters here.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
