[GitHub] [calcite] julianhyde closed pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
julianhyde closed pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty URL: https://github.com/apache/calcite/pull/2916 -- 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
[GitHub] [calcite] twdsilva commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
twdsilva commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r978970992
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -485,6 +516,54 @@ protected void perform(RelOptRuleCall call, @Nullable
Project topProject, RelNod
}
}
+ private static RelNode getViewWithFilter(RelNode view, @Nullable RexNode
viewFilter,
+ RelBuilder builder, @Nullable RexNode newPred) {
+RelNode viewWithFilter;
+if (viewFilter != null) {
+ viewWithFilter = builder.push(view).filter(viewFilter).build();
+} else if (newPred != null) {
+ viewWithFilter = builder.push(view).filter(newPred).build();
+} else {
+ viewWithFilter = builder.push(view).build();
+}
+return viewWithFilter;
+ }
+
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
+ * generate a filter on the view based on the query predicate so that the
view can be used.
+ */
+ private @Nullable Pair
generateViewPredicateAndFilter(RelNode view,
+ RelNode viewNode, RexBuilder rexBuilder, Pair
queryPreds) {
+if (viewNode instanceof Aggregate) {
+ Aggregate aggregate = (Aggregate) viewNode;
+ RelNode input = aggregate.getInput();
+ if (input instanceof Project) {
+Project project = (Project) input;
+int viewColumnIndex = 0;
+for (RexNode rexNode : project.getProjects()) {
+ if (rexNode instanceof RexCall) {
+RexCall rexCall = (RexCall) rexNode;
+if (rexCall.getOperator() instanceof SqlFloorFunction) {
Review Comment:
Updated the PR with this change, thanks for the review @sdreynolds.
--
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
[GitHub] [calcite] twdsilva commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
twdsilva commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r978970992
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -485,6 +516,54 @@ protected void perform(RelOptRuleCall call, @Nullable
Project topProject, RelNod
}
}
+ private static RelNode getViewWithFilter(RelNode view, @Nullable RexNode
viewFilter,
+ RelBuilder builder, @Nullable RexNode newPred) {
+RelNode viewWithFilter;
+if (viewFilter != null) {
+ viewWithFilter = builder.push(view).filter(viewFilter).build();
+} else if (newPred != null) {
+ viewWithFilter = builder.push(view).filter(newPred).build();
+} else {
+ viewWithFilter = builder.push(view).build();
+}
+return viewWithFilter;
+ }
+
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
+ * generate a filter on the view based on the query predicate so that the
view can be used.
+ */
+ private @Nullable Pair
generateViewPredicateAndFilter(RelNode view,
+ RelNode viewNode, RexBuilder rexBuilder, Pair
queryPreds) {
+if (viewNode instanceof Aggregate) {
+ Aggregate aggregate = (Aggregate) viewNode;
+ RelNode input = aggregate.getInput();
+ if (input instanceof Project) {
+Project project = (Project) input;
+int viewColumnIndex = 0;
+for (RexNode rexNode : project.getProjects()) {
+ if (rexNode instanceof RexCall) {
+RexCall rexCall = (RexCall) rexNode;
+if (rexCall.getOperator() instanceof SqlFloorFunction) {
Review Comment:
Update the PR with this change, thanks for the review @sdreynolds.
--
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
[GitHub] [calcite] dssysolyatin opened a new pull request, #2918: [CALCITE-5297] Casting dynamic variable twice throws exception
dssysolyatin opened a new pull request, #2918: URL: https://github.com/apache/calcite/pull/2918 It was originally intended that dynamic parameter is nullable by default (see SqlValidatorImpl#inferUnknownTypes), but DeriveTypeVisitor made dynamic parameter is nullable. In the end row type has not been preserved after converting query from SqlNode to 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. To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] zabetak closed pull request #2683: [CALCITE-4972] Qualify names for repeated structs in getFieldOrigins
zabetak closed pull request #2683: [CALCITE-4972] Qualify names for repeated structs in getFieldOrigins URL: https://github.com/apache/calcite/pull/2683 -- 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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r978374808
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -479,13 +481,24 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
@Override default PruneEmptyRule toRule() {
return new PruneEmptyRule(this) {
@Override public void onMatch(RelOptRuleCall call) {
- Join join = call.rel(0);
+ final Join join = call.rel(0);
+ final Values empty = call.rel(1);
+ final RelNode right = call.rel(2);
+ final RelBuilder b = call.builder();
Review Comment:
Renamed to `relBuilder`
--
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
[GitHub] [calcite] julianhyde commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
julianhyde commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r978348511
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -479,13 +481,24 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
@Override default PruneEmptyRule toRule() {
return new PruneEmptyRule(this) {
@Override public void onMatch(RelOptRuleCall call) {
- Join join = call.rel(0);
+ final Join join = call.rel(0);
+ final Values empty = call.rel(1);
+ final RelNode right = call.rel(2);
+ final RelBuilder b = call.builder();
Review Comment:
That's reasonable. I'll apply the change to my branch, and will merge
tomorrow when my batch of changes passes.
--
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
[GitHub] [calcite] asolimando commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
asolimando commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r978338926
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -479,13 +481,24 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
@Override default PruneEmptyRule toRule() {
return new PruneEmptyRule(this) {
@Override public void onMatch(RelOptRuleCall call) {
- Join join = call.rel(0);
+ final Join join = call.rel(0);
+ final Values empty = call.rel(1);
+ final RelNode right = call.rel(2);
+ final RelBuilder b = call.builder();
Review Comment:
Nitpick: in the codebase this is generally called `relBuilder`, the method
is not big but you might wonder what `b` is in the rest of the code, while with
`relBuilder` it will be pretty clear
--
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
[GitHub] [calcite] DeaconDesperado commented on pull request #2683: [CALCITE-4972] Qualify names for repeated structs in getFieldOrigins
DeaconDesperado commented on PR #2683: URL: https://github.com/apache/calcite/pull/2683#issuecomment-1255293057 Thank you @zabetak for the review! Checked in with 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. To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977595733
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,15 +554,13 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
- private static Stream getProjectStream(
- RelDataType inRowType,
- RelDataType castRowType,
- int castRowTypeOffset,
- RexBuilder rexBuilder) {
-return inRowType.getFieldList().stream().map(
-typeField -> rexBuilder.makeCast(
-castRowType.getFieldList().get(castRowTypeOffset +
typeField.getIndex()).getType(),
-rexBuilder.makeInputRef(typeField.getType(),
typeField.getIndex(;
+ private static Stream castToNullable(
+ RexBuilder rexBuilder, Stream leftExpressions) {
Review Comment:
fixed
--
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
[GitHub] [calcite] asolimando commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
asolimando commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977552984
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,15 +554,13 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
- private static Stream getProjectStream(
- RelDataType inRowType,
- RelDataType castRowType,
- int castRowTypeOffset,
- RexBuilder rexBuilder) {
-return inRowType.getFieldList().stream().map(
-typeField -> rexBuilder.makeCast(
-castRowType.getFieldList().get(castRowTypeOffset +
typeField.getIndex()).getType(),
-rexBuilder.makeInputRef(typeField.getType(),
typeField.getIndex(;
+ private static Stream castToNullable(
+ RexBuilder rexBuilder, Stream leftExpressions) {
Review Comment:
We probably want to use `expressions` instead of `leftExpressions` here,
since the method can be called on left or right, depending on the kind of outer
join.
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #2683: [CALCITE-4972] Qualify names for repeated structs in getFieldOrigins
zabetak commented on code in PR #2683:
URL: https://github.com/apache/calcite/pull/2683#discussion_r977435956
##
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##
@@ -9210,6 +9210,23 @@ public void _testGroupExpressionEquivalenceParams() {
+ " CATALOG.SALES.EMP.HIREDATE,"
+ " null,"
+ " null}"));
+
+sql("select e.empno from dept_nested, unnest(employees) as e")
+.assertFieldOrigin(
+is("{CATALOG.SALES.DEPT_NESTED.EMPLOYEES.EMPNO}"));
+
+sql("select * from UNNEST(ARRAY['a', 'b'])")
+.assertFieldOrigin(
+is("{null}"));
Review Comment:
nit: rather short so keep in everything in the same line
##
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##
@@ -9210,6 +9210,23 @@ public void _testGroupExpressionEquivalenceParams() {
+ " CATALOG.SALES.EMP.HIREDATE,"
+ " null,"
+ " null}"));
+
+sql("select e.empno from dept_nested, unnest(employees) as e")
+.assertFieldOrigin(
+is("{CATALOG.SALES.DEPT_NESTED.EMPLOYEES.EMPNO}"));
+
+sql("select * from UNNEST(ARRAY['a', 'b'])")
+.assertFieldOrigin(
+is("{null}"));
+
+sql("select * from UNNEST(ARRAY['a', 'b'], ARRAY['d', 'e'])")
+.assertFieldOrigin(
+is("{null, "
++ "null}"));
Review Comment:
nit: idem put in the same line
##
core/src/main/java/org/apache/calcite/sql/validate/UnnestNamespace.java:
##
@@ -64,6 +64,33 @@ class UnnestNamespace extends AbstractNamespace {
return null;
}
+ /**
+ * Given a field name from SelectScope, find the column in this
+ * UnnestNamespace it originates from.
+ *
+ * @param queryFieldName Name of column
+ * @return A SqlQualified if subfield comes from this unnest, null if not
found
+ */
+ public @Nullable SqlQualified getColumnUnnestedFrom(String queryFieldName) {
Review Comment:
Do we need to expose this as public? How about package private?
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977434703
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
Adding the cast only in case of full outer join enabled replacing the
`getProjectStream` method to a simpler `castToNullable` .
I kept `Stream.concat()` otherwise converting from `stream` to `iterable` is
required or if everything is `iterable` we loose the functional style in
`castToNullable` and `getNullLiteralStream`
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #2813: [CALCITE-5127] Error when executing query with subquery in select lis…
zabetak commented on code in PR #2813:
URL: https://github.com/apache/calcite/pull/2813#discussion_r977414870
##
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProjectRule.java:
##
@@ -42,6 +43,14 @@ protected EnumerableProjectRule(Config config) {
super(config);
}
+ @Override public boolean matches(RelOptRuleCall call) {
+LogicalProject project = call.rel(0);
+if (!project.getVariablesSet().isEmpty()) {
+ return false;
+}
+return true;
Review Comment:
Replace with `return project.getVariablesSet().isEmpty();` Idem to the other
rules.
##
core/src/test/java/org/apache/calcite/plan/RelWriterTest.java:
##
@@ -998,6 +999,21 @@ void testCorrelateQuery(SqlExplainFormat format) {
.assertThatPlan(isLinux(expected));
}
+ @Test void testProjectionWithCorrelationVaribles() {
+final Function relFn = b -> b.scan("EMP")
+.project(
+ImmutableList.of(b.field("ENAME")),
+ImmutableList.of("ename"),
+true,
+ImmutableSet.of(b.getCluster().createCorrel()))
+.build();
+
+final String expected = "LogicalProject(variablesSet=[[$cor0]],
ename=[$1])\n"
++ " LogicalTableScan(table=[[scott, EMP]])\n";
Review Comment:
Agreed!
##
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##
@@ -1817,7 +1817,24 @@ public RelBuilder project(Iterable
nodes,
*/
public RelBuilder project(Iterable nodes,
Iterable fieldNames, boolean force) {
-return project_(nodes, fieldNames, ImmutableList.of(), force);
+return project(nodes, fieldNames, force, ImmutableSet.of());
+ }
+
+ /**
+ * The same with {@link #project(Iterable, Iterable, boolean)}, with
additional
+ * variablesSet param.
+ *
+ * @param nodes Expressions
+ * @param fieldNames Suggested field names
+ * @param force create project even if it is identity
+ * @param variablesSet Correlating variables that are set when reading a row
+ * from the input, and which may be referenced from the
+ * projection expressions
+ */
+ public RelBuilder project(Iterable nodes,
+ Iterable fieldNames, boolean force,
+ Iterable variablesSet) {
+return project_(nodes, fieldNames, ImmutableList.of(), force,
variablesSet);
Review Comment:
There are certain disadvantages with the approach I suggested. We can take
advantage of `variablesSet` in various places such as `RelFieldTrimmer` so
overall it makes sense to move forward with this change. Marking this as
resovled.
--
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
[GitHub] [calcite] julianhyde commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
julianhyde commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977348821
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
I have a feeling that it could be even shorter if you use Guava's
`List.concat()` rather than `Stream.concat()`, and use `RelBuilder.fields()`.
You'll have to assign the `RelBuilder` to a variable.
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977335384
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
It works, thanks for the hint.
--
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
[GitHub] [calcite] julianhyde commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
julianhyde commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977326394
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
What if you replace `Pair.right(projects)` with
`join.getRowType().getFieldNames()`?
--
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
[GitHub] [calcite] bchapuis opened a new pull request, #2917: [CALCITE-5280] Add geometry aggregate functions
bchapuis opened a new pull request, #2917:
URL: https://github.com/apache/calcite/pull/2917
This PR addresses
[CALCITE-5280](https://issues.apache.org/jira/browse/CALCITE-5280).
Notice that I had to remove one of the existing ST_UNION function (due to a
name clash?). Also, this function is not implemented in
[postgis](https://postgis.net/docs/ST_Union.html).
```
@SemiStrict public static Geometry ST_Union(Geometry geomCollection) {
return geomCollection.union();
}
```
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977311883
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
The field names can be inherited from the left branch but it has to be set
for the `NULL` literals otherwise names like `$f9` are generated.
expected
```
LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],
SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], DEPTNO0=[$9], NAME=[$10])
LogicalProject(EMPNO=[CAST($0):INTEGER], ENAME=[CAST($1):VARCHAR(20)],
JOB=[CAST($2):VARCHAR(10)], MGR=[$3], HIREDATE=[CAST($4):TIMESTAMP(0)],
SAL=[CAST($5):INTEGER], COMM=[CAST($6):INTEGER], DEPTNO=[CAST($7):INTEGER],
SLACKER=[CAST($8):BOOLEAN], DEPTNO0=[null:INTEGER], NAME=[null:VARCHAR(10)])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
```
actual
```
LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],
SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], DEPTNO0=[$9], NAME=[$10])
LogicalProject(EMPNO=[CAST($0):INTEGER], ENAME=[CAST($1):VARCHAR(20)],
JOB=[CAST($2):VARCHAR(10)], MGR=[$3], HIREDATE=[CAST($4):TIMESTAMP(0)],
SAL=[CAST($5):INTEGER], COMM=[CAST($6):INTEGER], DEPTNO=[CAST($7):INTEGER],
SLACKER=[CAST($8):BOOLEAN], $f9=[null:INTEGER], $f10=[null:VARCHAR(10)])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
```
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977311883
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
The field names can be inherited from the left branch but it has to be set
for the `NULL` literals otherwise names like `$f9` are generated.
explected
```
LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],
SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], DEPTNO0=[$9], NAME=[$10])
LogicalProject(EMPNO=[CAST($0):INTEGER], ENAME=[CAST($1):VARCHAR(20)],
JOB=[CAST($2):VARCHAR(10)], MGR=[$3], HIREDATE=[CAST($4):TIMESTAMP(0)],
SAL=[CAST($5):INTEGER], COMM=[CAST($6):INTEGER], DEPTNO=[CAST($7):INTEGER],
SLACKER=[CAST($8):BOOLEAN], DEPTNO0=[null:INTEGER], NAME=[null:VARCHAR(10)])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
```
actual
```
LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],
SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], DEPTNO0=[$9], NAME=[$10])
LogicalProject(EMPNO=[CAST($0):INTEGER], ENAME=[CAST($1):VARCHAR(20)],
JOB=[CAST($2):VARCHAR(10)], MGR=[$3], HIREDATE=[CAST($4):TIMESTAMP(0)],
SAL=[CAST($5):INTEGER], COMM=[CAST($6):INTEGER], DEPTNO=[CAST($7):INTEGER],
SLACKER=[CAST($8):BOOLEAN], $f9=[null:INTEGER], $f10=[null:VARCHAR(10)])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
```
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977304390
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
Sorry, just read your comment, let me try it.
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977303067
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -480,9 +486,25 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
return new PruneEmptyRule(this) {
@Override public void onMatch(RelOptRuleCall call) {
Join join = call.rel(0);
+ Values empty = call.rel(1);
+ RelNode right = call.rel(2);
+ RexBuilder rexBuilder = call.builder().getRexBuilder();
if (join.getJoinType().generatesNullsOnLeft()) {
// "select * from emp right join dept" is not necessarily empty if
// emp is empty
+// join can be removed and take the right branch, all columns come
from emp are null
+List projects = new ArrayList<>(
+empty.getRowType().getFieldCount() +
right.getRowType().getFieldCount());
+List columnNames = new ArrayList<>(
+empty.getRowType().getFieldCount() +
right.getRowType().getFieldCount());
+// left
+addNullLiterals(rexBuilder, empty, projects, columnNames);
+// right
+copyProjects(rexBuilder, right.getRowType(),
+join.getRowType(), empty.getRowType().getFieldCount(),
projects, columnNames);
+
+RelNode project = call.builder().push(right).project(projects,
columnNames).build();
+call.transformTo(project);
Review Comment:
Refactored the utility methods to use the Java stream api. I defined stream
of List> as a return type to ensure the order of expressions.
If this is not what you mean could you please give some example?
--
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
[GitHub] [calcite] julianhyde commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
julianhyde commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977290395
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -551,17 +529,15 @@ public interface JoinRightEmptyRuleConfig extends
PruneEmptyRule.Config {
// "select * from emp left join dept" is not necessarily empty if
// dept is empty
// join can be removed and take the left branch, all columns come
from dept are null
-List projects = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-List columnNames = new ArrayList<>(
-left.getRowType().getFieldCount() +
empty.getRowType().getFieldCount());
-// left
-copyProjects(
-rexBuilder, left.getRowType(), join.getRowType(), 0, projects,
columnNames);
-// right
-addNullLiterals(rexBuilder, empty, projects, columnNames);
-
-RelNode project = call.builder().push(left).project(projects,
columnNames).build();
+List> projects =
+Stream.concat(
+getProjectStream(left.getRowType(), join.getRowType(), 0,
rexBuilder),
+getNullLiteralStream(empty,
rexBuilder)).collect(Collectors.toList());
+
+RelNode project = call.builder()
+.push(left)
+.project(Pair.left(projects), Pair.right(projects))
+.build();
Review Comment:
I believe that the field names are the same before and after. If so, you
don't need to create pairs of (RexNode, String), just RexNode. Which I think
would simplify getProjectStream and getNullLiteralStream.
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977285954
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -491,6 +513,29 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
+ private static void addNullLiterals(RexBuilder rexBuilder, Values empty,
+ List projectFields, List newColumnNames) {
+for (int i = 0; i < empty.getRowType().getFieldList().size(); ++i) {
+ RelDataTypeField relDataTypeField =
empty.getRowType().getFieldList().get(i);
+ RexNode nullLiteral =
rexBuilder.makeNullLiteral(relDataTypeField.getType());
+ projectFields.add(nullLiteral);
+ newColumnNames.add(empty.getRowType().getFieldList().get(i).getName());
+}
+ }
+
+ public static void copyProjects(RexBuilder rexBuilder, RelDataType inRowType,
Review Comment:
No it should be private so I changed. Thanks.
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r977285031
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -491,6 +513,29 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
+ private static void addNullLiterals(RexBuilder rexBuilder, Values empty,
+ List projectFields, List newColumnNames) {
+for (int i = 0; i < empty.getRowType().getFieldList().size(); ++i) {
+ RelDataTypeField relDataTypeField =
empty.getRowType().getFieldList().get(i);
+ RexNode nullLiteral =
rexBuilder.makeNullLiteral(relDataTypeField.getType());
+ projectFields.add(nullLiteral);
+ newColumnNames.add(empty.getRowType().getFieldList().get(i).getName());
+}
+ }
+
+ public static void copyProjects(RexBuilder rexBuilder, RelDataType inRowType,
+ RelDataType castRowType, int castRowTypeOffset,
+ List outProjects, List outProjectNames) {
+for (int i = 0; i < inRowType.getFieldCount(); ++i) {
+ RelDataTypeField relDataTypeField = inRowType.getFieldList().get(i);
+ RexInputRef inputRef =
rexBuilder.makeInputRef(relDataTypeField.getType(), i);
+ RexNode cast = rexBuilder.makeCast(
+ castRowType.getFieldList().get(castRowTypeOffset + i).getType(),
inputRef);
+ outProjects.add(cast);
+ outProjectNames.add(relDataTypeField.getName());
+}
+ }
+
Review Comment:
Renamed, refactored the methods and also moved after 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.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] zabetak closed pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
zabetak closed pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules URL: https://github.com/apache/calcite/pull/2915 -- 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
[GitHub] [calcite] julianhyde commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
julianhyde commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r976851396
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -480,9 +486,25 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
return new PruneEmptyRule(this) {
@Override public void onMatch(RelOptRuleCall call) {
Join join = call.rel(0);
+ Values empty = call.rel(1);
+ RelNode right = call.rel(2);
+ RexBuilder rexBuilder = call.builder().getRexBuilder();
if (join.getJoinType().generatesNullsOnLeft()) {
// "select * from emp right join dept" is not necessarily empty if
// emp is empty
+// join can be removed and take the right branch, all columns come
from emp are null
+List projects = new ArrayList<>(
+empty.getRowType().getFieldCount() +
right.getRowType().getFieldCount());
+List columnNames = new ArrayList<>(
+empty.getRowType().getFieldCount() +
right.getRowType().getFieldCount());
+// left
+addNullLiterals(rexBuilder, empty, projects, columnNames);
+// right
+copyProjects(rexBuilder, right.getRowType(),
+join.getRowType(), empty.getRowType().getFieldCount(),
projects, columnNames);
+
+RelNode project = call.builder().push(right).project(projects,
columnNames).build();
+call.transformTo(project);
Review Comment:
Can this be written in a functional style? Rather than creating empty lists
then filling them, use `map`. `RelBuilder` has plenty of methods that return
lists to types, lists of names, etc.
--
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
[GitHub] [calcite] kasakrisz commented on pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on PR #2916: URL: https://github.com/apache/calcite/pull/2916#issuecomment-1253858830 Should this patch also be mentioned in `history.md` in ` Bug-fixes, API changes and minor enhancements` section ? -- 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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
kasakrisz commented on code in PR #2915:
URL: https://github.com/apache/calcite/pull/2915#discussion_r976643211
##
site/_docs/history.md:
##
@@ -44,6 +44,10 @@ z.
Breaking Changes
{: #breaking-1-33-0}
+* [https://issues.apache.org/jira/browse/CALCITE-5293;>CALCITE-5293]
+ Support general set operators in PruneEmptyRules. The default configuration
of PruneEmptyRules for Set operators is changed: the rules matching scope has
increased.
Review Comment:
fixed
--
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
[GitHub] [calcite] asolimando commented on a diff in pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
asolimando commented on code in PR #2915:
URL: https://github.com/apache/calcite/pull/2915#discussion_r976634022
##
site/_docs/history.md:
##
@@ -44,6 +44,10 @@ z.
Breaking Changes
{: #breaking-1-33-0}
+* [https://issues.apache.org/jira/browse/CALCITE-5293;>CALCITE-5293]
+ Support general set operators in PruneEmptyRules. The default configuration
of PruneEmptyRules for Set operators is changed: the rules matching scope has
increased.
Review Comment:
Nitpick:
```suggestion
Support general set operators in PruneEmptyRules. The default
configuration of PruneEmptyRules for Set operators has changed: the rules
matching scope has increased.
```
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
kasakrisz commented on code in PR #2915:
URL: https://github.com/apache/calcite/pull/2915#discussion_r976631083
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -94,7 +94,7 @@ public interface Config extends RelRule.Config {
public static final RelOptRule UNION_INSTANCE =
ImmutableUnionEmptyPruneRuleConfig.of()
.withOperandSupplier(b0 ->
- b0.operand(LogicalUnion.class).unorderedInputs(b1 ->
+ b0.operand(Union.class).unorderedInputs(b1 ->
Review Comment:
Reverted the revert :smile: and updated history.md
--
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
[GitHub] [calcite] asolimando commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
asolimando commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r976611598
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -491,6 +513,29 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
+ private static void addNullLiterals(RexBuilder rexBuilder, Values empty,
+ List projectFields, List newColumnNames) {
+for (int i = 0; i < empty.getRowType().getFieldList().size(); ++i) {
+ RelDataTypeField relDataTypeField =
empty.getRowType().getFieldList().get(i);
+ RexNode nullLiteral =
rexBuilder.makeNullLiteral(relDataTypeField.getType());
+ projectFields.add(nullLiteral);
+ newColumnNames.add(empty.getRowType().getFieldList().get(i).getName());
+}
+ }
+
+ public static void copyProjects(RexBuilder rexBuilder, RelDataType inRowType,
Review Comment:
Does it need to be `public`?
--
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
[GitHub] [calcite] asolimando commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
asolimando commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r976611160
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -491,6 +513,29 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
+ private static void addNullLiterals(RexBuilder rexBuilder, Values empty,
+ List projectFields, List newColumnNames) {
+for (int i = 0; i < empty.getRowType().getFieldList().size(); ++i) {
+ RelDataTypeField relDataTypeField =
empty.getRowType().getFieldList().get(i);
+ RexNode nullLiteral =
rexBuilder.makeNullLiteral(relDataTypeField.getType());
+ projectFields.add(nullLiteral);
+ newColumnNames.add(empty.getRowType().getFieldList().get(i).getName());
+}
+ }
+
+ public static void copyProjects(RexBuilder rexBuilder, RelDataType inRowType,
+ RelDataType castRowType, int castRowTypeOffset,
+ List outProjects, List outProjectNames) {
+for (int i = 0; i < inRowType.getFieldCount(); ++i) {
+ RelDataTypeField relDataTypeField = inRowType.getFieldList().get(i);
+ RexInputRef inputRef =
rexBuilder.makeInputRef(relDataTypeField.getType(), i);
+ RexNode cast = rexBuilder.makeCast(
+ castRowType.getFieldList().get(castRowTypeOffset + i).getType(),
inputRef);
+ outProjects.add(cast);
+ outProjectNames.add(relDataTypeField.getName());
+}
+ }
+
Review Comment:
As highlighted in the corresponding Hive PR, since those two methods are
used by both rules, it would be better to move them either before them, or
after them, rather than having them in between the rules as of now.
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
zabetak commented on code in PR #2915:
URL: https://github.com/apache/calcite/pull/2915#discussion_r976589860
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -94,7 +94,7 @@ public interface Config extends RelRule.Config {
public static final RelOptRule UNION_INSTANCE =
ImmutableUnionEmptyPruneRuleConfig.of()
.withOperandSupplier(b0 ->
- b0.operand(LogicalUnion.class).unorderedInputs(b1 ->
+ b0.operand(Union.class).unorderedInputs(b1 ->
Review Comment:
Hmm, I didn't notice that the other rules were configured for the general
operators. Your initial proposal makes sense. Please revert
https://github.com/apache/calcite/pull/2915/commits/ad4cfeaf49100f0ce7f5668c0c55fc2fbd8a0a8a
and add a note in `history.md` (breaking changes section) about the slight
change in behavior in the default configuration regarding these rules.
I will merge the PR afterwards; sorry for the back n forth.
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
zabetak commented on code in PR #2915:
URL: https://github.com/apache/calcite/pull/2915#discussion_r976589860
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -94,7 +94,7 @@ public interface Config extends RelRule.Config {
public static final RelOptRule UNION_INSTANCE =
ImmutableUnionEmptyPruneRuleConfig.of()
.withOperandSupplier(b0 ->
- b0.operand(LogicalUnion.class).unorderedInputs(b1 ->
+ b0.operand(Union.class).unorderedInputs(b1 ->
Review Comment:
Hmm, I didn't notice that the other rules were configured for the general
operators. Your initial proposal makes sense. Please revert
https://github.com/apache/calcite/pull/2915/commits/ad4cfeaf49100f0ce7f5668c0c55fc2fbd8a0a8a
and add a note in `history.md` about the slight change in behavior in the
default configuration regarding these rules.
I will merge the PR afterwards; sorry for the back n forth.
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r976576543
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -491,6 +512,30 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
+ private static void addNullLiterals(RexBuilder rexBuilder, Values empty,
+ List projectFields, List newColumnNames) {
+for (int i = 0; i < empty.getRowType().getFieldList().size(); ++i) {
+ RelDataTypeField relDataTypeField =
empty.getRowType().getFieldList().get(i);
+ RexNode nullLiteral =
rexBuilder.makeNullLiteral(relDataTypeField.getType());
+ projectFields.add(nullLiteral);
+ newColumnNames.add(empty.getRowType().getFieldList().get(i).getName());
+}
+ }
+
+ public static void copyProjects(RexBuilder rexBuilder, RelDataType inRowType,
+ RelDataType castRowType, int castRowTypeOffset,
+ List outProjects, List outProjectNames) {
+
Review Comment:
removed
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
kasakrisz commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r976576290
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -480,9 +486,24 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
return new PruneEmptyRule(this) {
@Override public void onMatch(RelOptRuleCall call) {
Join join = call.rel(0);
+ Values empty = call.rel(1);
+ RelNode right = call.rel(2);
+ RexBuilder rexBuilder = call.builder().getRexBuilder();
if (join.getJoinType().generatesNullsOnLeft()) {
// "select * from emp right join dept" is not necessarily empty if
Review Comment:
Updated the comment.
--
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
[GitHub] [calcite] kasakrisz commented on a diff in pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
kasakrisz commented on code in PR #2915:
URL: https://github.com/apache/calcite/pull/2915#discussion_r976575571
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -94,7 +94,7 @@ public interface Config extends RelRule.Config {
public static final RelOptRule UNION_INSTANCE =
ImmutableUnionEmptyPruneRuleConfig.of()
.withOperandSupplier(b0 ->
- b0.operand(LogicalUnion.class).unorderedInputs(b1 ->
+ b0.operand(Union.class).unorderedInputs(b1 ->
Review Comment:
Only set operator rules configured for Logical operators so it makes a bit
inconsistent the default config.
Anyway I got your point and reverted.
--
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
[GitHub] [calcite] rubenada commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
rubenada commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r976517058
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -491,6 +512,30 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
}
}
+ private static void addNullLiterals(RexBuilder rexBuilder, Values empty,
+ List projectFields, List newColumnNames) {
+for (int i = 0; i < empty.getRowType().getFieldList().size(); ++i) {
+ RelDataTypeField relDataTypeField =
empty.getRowType().getFieldList().get(i);
+ RexNode nullLiteral =
rexBuilder.makeNullLiteral(relDataTypeField.getType());
+ projectFields.add(nullLiteral);
+ newColumnNames.add(empty.getRowType().getFieldList().get(i).getName());
+}
+ }
+
+ public static void copyProjects(RexBuilder rexBuilder, RelDataType inRowType,
+ RelDataType castRowType, int castRowTypeOffset,
+ List outProjects, List outProjectNames) {
+
Review Comment:
Nitpick: could you please remove this empty line to keep it consistent with
the rest of 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.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] rubenada commented on a diff in pull request #2916: [CALCITE-5294] Improve PruneEmptyRules join instances to remove join if generates null and one branch is empty
rubenada commented on code in PR #2916:
URL: https://github.com/apache/calcite/pull/2916#discussion_r976516051
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -480,9 +486,24 @@ public interface JoinLeftEmptyRuleConfig extends
PruneEmptyRule.Config {
return new PruneEmptyRule(this) {
@Override public void onMatch(RelOptRuleCall call) {
Join join = call.rel(0);
+ Values empty = call.rel(1);
+ RelNode right = call.rel(2);
+ RexBuilder rexBuilder = call.builder().getRexBuilder();
if (join.getJoinType().generatesNullsOnLeft()) {
// "select * from emp right join dept" is not necessarily empty if
Review Comment:
Probably this comment (and the analogous one in `JoinRightEmptyRuleConfig`)
needs to be adjusted because of this 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.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] zabetak commented on a diff in pull request #2915: [CALCITE-5293] Support general set operators in PruneEmptyRules
zabetak commented on code in PR #2915:
URL: https://github.com/apache/calcite/pull/2915#discussion_r976486206
##
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##
@@ -94,7 +94,7 @@ public interface Config extends RelRule.Config {
public static final RelOptRule UNION_INSTANCE =
ImmutableUnionEmptyPruneRuleConfig.of()
.withOperandSupplier(b0 ->
- b0.operand(LogicalUnion.class).unorderedInputs(b1 ->
+ b0.operand(Union.class).unorderedInputs(b1 ->
Review Comment:
Better leave the default configurations unchanged to avoid accidentally
breaking other projects. Projects who wish to enlarge the matching scope can
modify the configuration.
--
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
[GitHub] [calcite] tjbanghart commented on a diff in pull request #2913: [CALCITE-5180] Implement BigQuery TIME_TRUNC and TIMESTAMP_TRUNC functions
tjbanghart commented on code in PR #2913:
URL: https://github.com/apache/calcite/pull/2913#discussion_r975975647
##
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##
@@ -4083,6 +4083,55 @@ void subTestIntervalSecondNegative() {
.fails("(?s).*Was expecting one of.*");
}
+ @Test void testTimestampTrunc() {
+List timeUnits = ImmutableList.builder()
+.add("MINUTE")
+.add("HOUR")
+.add("DAY")
+.add("WEEK")
+.add("MONTH")
+.add("QUARTER")
+.add("YEAR")
+.add("ISOYEAR")
+.build();
+
+final SqlOperatorTable opTable = operatorTableFor(SqlLibrary.BIG_QUERY);
+String test = "TIMESTAMP_TRUNC(TIMESTAMP '2000-01-01 01:00:00', %s)";
+
+for (String unit : timeUnits) {
+ wholeExpr(String.format(Locale.ROOT, test, unit))
+ .fails("No match found for function signature "
+ + "TIMESTAMP_TRUNC\\(,.*\\)");
+ expr(String.format(Locale.ROOT, test, unit))
+ .withOperatorTable(opTable)
+ .columnType("TIMESTAMP(0) NOT NULL");
+}
+ }
+
+ @Test void testTimeTrunc() {
+List timeUnits = ImmutableList.builder()
+.add("MILLISECOND")
+.add("SECOND")
+.add("MINUTE")
+.add("HOUR")
+.build();
+
+final SqlOperatorTable opTable = operatorTableFor(SqlLibrary.BIG_QUERY);
+String test = "TIME_TRUNC(TIME '15:30:00.00', %s)";
+
+for (String unit : timeUnits) {
+ wholeExpr(String.format(Locale.ROOT, test, unit))
+ .fails("No match found for function signature "
+ + "TIME_TRUNC\\(,.*\\)");
+ expr(String.format(Locale.ROOT, test, unit))
+ .withOperatorTable(opTable)
+ .columnType("TIME(0) NOT NULL");
+}
+// should fail on incompatible time unit
+expr("TIME_TRUNC(TIME '15:30:00.00', ^DAY^)")
+.fails("(?s).*Was expecting one of.*");
+ }
+
Review Comment:
These can probably be simplified to run a single time unit but they validate
for an error message if the BigQuery operator table is not used. I can add
similar check in `SqlOperatorTest` so this file isn't touched.
##
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##
@@ -520,7 +520,9 @@ public class SqlParserTest {
"TEMPORARY", "92", "99",
"THEN", "92", "99", "2003", "2011", "2014", "c",
"TIME", "92", "99", "2003", "2011", "2014", "c",
+ "TIME_TRUNC", // BigQuery
Review Comment:
This is me following an example commit too closely.
--
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
[GitHub] [calcite] julianhyde commented on a diff in pull request #2913: [CALCITE-5180] Implement BigQuery TIME_TRUNC and TIMESTAMP_TRUNC functions
julianhyde commented on code in PR #2913:
URL: https://github.com/apache/calcite/pull/2913#discussion_r975915626
##
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##
@@ -520,7 +520,9 @@ public class SqlParserTest {
"TEMPORARY", "92", "99",
"THEN", "92", "99", "2003", "2011", "2014", "c",
"TIME", "92", "99", "2003", "2011", "2014", "c",
+ "TIME_TRUNC", // BigQuery
Review Comment:
I'm surprised there are new keywords. We needed a new keyword for `ILIKE`
because it's infix, but these can just be function names.
##
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##
@@ -4083,6 +4083,55 @@ void subTestIntervalSecondNegative() {
.fails("(?s).*Was expecting one of.*");
}
+ @Test void testTimestampTrunc() {
+List timeUnits = ImmutableList.builder()
+.add("MINUTE")
+.add("HOUR")
+.add("DAY")
+.add("WEEK")
+.add("MONTH")
+.add("QUARTER")
+.add("YEAR")
+.add("ISOYEAR")
+.build();
+
+final SqlOperatorTable opTable = operatorTableFor(SqlLibrary.BIG_QUERY);
+String test = "TIMESTAMP_TRUNC(TIMESTAMP '2000-01-01 01:00:00', %s)";
+
+for (String unit : timeUnits) {
+ wholeExpr(String.format(Locale.ROOT, test, unit))
+ .fails("No match found for function signature "
+ + "TIMESTAMP_TRUNC\\(,.*\\)");
+ expr(String.format(Locale.ROOT, test, unit))
+ .withOperatorTable(opTable)
+ .columnType("TIMESTAMP(0) NOT NULL");
+}
+ }
+
+ @Test void testTimeTrunc() {
+List timeUnits = ImmutableList.builder()
+.add("MILLISECOND")
+.add("SECOND")
+.add("MINUTE")
+.add("HOUR")
+.build();
+
+final SqlOperatorTable opTable = operatorTableFor(SqlLibrary.BIG_QUERY);
+String test = "TIME_TRUNC(TIME '15:30:00.00', %s)";
+
+for (String unit : timeUnits) {
+ wholeExpr(String.format(Locale.ROOT, test, unit))
+ .fails("No match found for function signature "
+ + "TIME_TRUNC\\(,.*\\)");
+ expr(String.format(Locale.ROOT, test, unit))
+ .withOperatorTable(opTable)
+ .columnType("TIME(0) NOT NULL");
+}
+// should fail on incompatible time unit
+expr("TIME_TRUNC(TIME '15:30:00.00', ^DAY^)")
+.fails("(?s).*Was expecting one of.*");
+ }
+
Review Comment:
Are these tests superfluous? between `SqlParserTest` and `SqlOperatorTest` I
think you have it covered. I don't think there's any non-trivial validation
behavior.
##
site/_docs/reference.md:
##
@@ -2625,6 +2625,8 @@ semantics.
| b m o p | SUBSTR(string, position [, substringLength ]) | Returns a portion
of *string*, beginning at character *position*, *substringLength* characters
long. SUBSTR calculates lengths using characters as defined by the input
character set
| m | STRCMP(string, string) | Returns 0 if both of
the strings are same and returns -1 when the first argument is smaller than the
second and 1 when the second one is smaller than the first one
| o | TANH(numeric) | Returns the hyperbolic
tangent of *numeric*
+| b | TIME_TRUNC(time, timeUnit) | Truncates a *time*
value to the granularity of *timeUnit*. The *time* value is always rounded to
the beginning of timeUnit, which can be one of the following: MILLISECOND,
SECOND, MINUTE, HOUR.
+| b | TIMESTAMP_TRUNC(timestamp, timeUnit) | Truncates a *timestamp*
value to the granularity of *timeUnit*. The *timestamp* value is always rounded
to the beginning of the *timeUnit*.
Review Comment:
move down a few lines, to retain alphabetical order
##
babel/src/test/resources/sql/big-query.iq:
##
@@ -16,7 +16,809 @@
# limitations under the License.
#
!use scott-big-query
-!set outputformat csv
+!set outputformat mysql
Review Comment:
when I merge this I'll factor out the big-query.iq change as a separate
commit
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -1604,6 +1604,10 @@ protected static Calendar getCalendarNotTooNear(int
timeUnit) {
f.checkScalar("{fn TIMESTAMPDIFF(MONTH,"
+ " TIMESTAMP '2019-09-01 00:00:00',"
+ " TIMESTAMP '2020-03-01 00:00:00')}", "6", "INTEGER NOT NULL");
+f.checkScalar("{fn TIMESTAMP_TRUNC(TIMESTAMP '2019-09-20 15:30:00', MONTH)
}",
Review Comment:
I don't think JDBC syntax is a requirement. You should remove these lines,
and revert the changes to `SqlJdbcFunctionCall.java`.
--
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,
[GitHub] [calcite] libenchao commented on a diff in pull request #2911: [CALCITE-5127] Error when executing query with subquery in select lis…
libenchao commented on code in PR #2911:
URL: https://github.com/apache/calcite/pull/2911#discussion_r975277091
##
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##
@@ -481,6 +481,18 @@ public TrimResult trimFields(
final int fieldCount = rowType.getFieldCount();
final RelNode input = project.getInput();
+boolean containsSubQuery = false;
+for (RexNode node : project.getProjects()) {
+ if (RexUtil.containsSubQuery(node)) {
+containsSubQuery = true;
+break;
+ }
+}
+// Do not trim Project's fields before SubQueryRemoveRule applies.
+if (containsSubQuery) {
+ return result(project, Mappings.createIdentity(fieldCount));
+}
+
Review Comment:
tests added.
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #2911: [CALCITE-5127] Error when executing query with subquery in select lis…
zabetak commented on code in PR #2911:
URL: https://github.com/apache/calcite/pull/2911#discussion_r975066157
##
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##
@@ -481,6 +481,18 @@ public TrimResult trimFields(
final int fieldCount = rowType.getFieldCount();
final RelNode input = project.getInput();
+boolean containsSubQuery = false;
+for (RexNode node : project.getProjects()) {
+ if (RexUtil.containsSubQuery(node)) {
+containsSubQuery = true;
+break;
+ }
+}
+// Do not trim Project's fields before SubQueryRemoveRule applies.
+if (containsSubQuery) {
+ return result(project, Mappings.createIdentity(fieldCount));
+}
+
Review Comment:
Maybe it is worth adding the new test also in `RelFieldTrimmerTest` since it
is explicitly targeting this code path.
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #2911: [CALCITE-5127] Error when executing query with subquery in select lis…
zabetak commented on code in PR #2911:
URL: https://github.com/apache/calcite/pull/2911#discussion_r975061171
##
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##
@@ -481,6 +481,18 @@ public TrimResult trimFields(
final int fieldCount = rowType.getFieldCount();
final RelNode input = project.getInput();
+boolean containsSubQuery = false;
+for (RexNode node : project.getProjects()) {
+ if (RexUtil.containsSubQuery(node)) {
+containsSubQuery = true;
+break;
+ }
+}
+// Do not trim Project's fields before SubQueryRemoveRule applies.
+if (containsSubQuery) {
+ return result(project, Mappings.createIdentity(fieldCount));
+}
+
Review Comment:
I didn't notice that the new test in `sub-query.iq` was passing from here,
great! Since there are tests then I guess it is OK to keep the changes as part
of this PR.
> If there is a test case which is affected by this change, then it was
already wrong.
It may be wrong but not always, right? This is a rough check as we discussed
not precise.
Moreover if there is a sub-query in the top project then field trimmer will
stop without doing anything. I think we can do better than that. Is it valid to
trim its input? I mean something like the following:
```java
if (containsSubQuery) {
return trimFields((RelNode) project, fieldsUsed, extraFields);
}
```
--
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
[GitHub] [calcite] alex-plekhanov commented on a diff in pull request #2912: [CALCITE-5288] '(a > 1 and a < 3) or (a > 2 and a < 4)' can't be simplified
alex-plekhanov commented on code in PR #2912:
URL: https://github.com/apache/calcite/pull/2912#discussion_r974920016
##
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##
@@ -1243,6 +1243,13 @@ private void checkExponentialCnf(int n) {
RelOptPredicateList.of(rexBuilder,
ImmutableList.of(le(aRef, literal(5)), le(bRef, literal(5,
"false");
+
+// condition "(a >= 1 and a <= 3) or (a >= 2 and a <= 4)
+// yelds "a >= 1 and a <= 4"
+checkSimplifyFilter(
+or(and(ge(aRef, literal(1)), le(aRef, literal(3))),
+and(ge(aRef, literal(2)), le(aRef, literal(4,
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.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2911: [CALCITE-5127] Error when executing query with subquery in select lis…
libenchao commented on code in PR #2911:
URL: https://github.com/apache/calcite/pull/2911#discussion_r974896291
##
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##
@@ -481,6 +481,18 @@ public TrimResult trimFields(
final int fieldCount = rowType.getFieldCount();
final RelNode input = project.getInput();
+boolean containsSubQuery = false;
+for (RexNode node : project.getProjects()) {
+ if (RexUtil.containsSubQuery(node)) {
+containsSubQuery = true;
+break;
+ }
+}
+// Do not trim Project's fields before SubQueryRemoveRule applies.
+if (containsSubQuery) {
+ return result(project, Mappings.createIdentity(fieldCount));
+}
+
Review Comment:
The test added in `sub-query.iq` will fail without this change. It surprised
me too that this does not affect any existing plan tests. I guess that's
because we don't have enough plan tests for sub query. If there is a test case
which is affected by this change, then it was already wrong.
Due to the integrity of this issue I think this is ok to keep it in the same
PR, 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.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #2913: [CALCITE-5180] Implement BigQuery TIME_TRUNC and TIMESTAMP_TRUNC functions
tjbanghart commented on code in PR #2913:
URL: https://github.com/apache/calcite/pull/2913#discussion_r974859878
##
core/src/main/java/org/apache/calcite/prepare/CalciteCatalogReader.java:
##
@@ -91,7 +91,10 @@ public class CalciteCatalogReader implements
Prepare.CatalogReader {
public CalciteCatalogReader(CalciteSchema rootSchema,
List defaultSchema, RelDataTypeFactory typeFactory,
CalciteConnectionConfig config) {
-this(rootSchema, SqlNameMatchers.withCaseSensitive(config != null &&
config.caseSensitive()),
+this(rootSchema,
+// TODO: BigQuery has case-insensitive functions
Review Comment:
https://github.com/apache/calcite/pull/2914 should fix this.
--
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
[GitHub] [calcite] chunweilei commented on a diff in pull request #2912: [CALCITE-5288] '(a > 1 and a < 3) or (a > 2 and a < 4)' can't be simplified
chunweilei commented on code in PR #2912:
URL: https://github.com/apache/calcite/pull/2912#discussion_r974817088
##
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##
@@ -1243,6 +1243,13 @@ private void checkExponentialCnf(int n) {
RelOptPredicateList.of(rexBuilder,
ImmutableList.of(le(aRef, literal(5)), le(bRef, literal(5,
"false");
+
+// condition "(a >= 1 and a <= 3) or (a >= 2 and a <= 4)
+// yelds "a >= 1 and a <= 4"
+checkSimplifyFilter(
+or(and(ge(aRef, literal(1)), le(aRef, literal(3))),
+and(ge(aRef, literal(2)), le(aRef, literal(4,
Review Comment:
Could you add a test like "(a >= 1 and a <= 3) or (a >= 0 and a <= 2) ->
a>=0 and a<=3 "?
--
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
[GitHub] [calcite] chunweilei commented on a diff in pull request #2891: [CALCITE-5260] Pop the last RelNode added into the bindings list when failing to apply a rule
chunweilei commented on code in PR #2891:
URL: https://github.com/apache/calcite/pull/2891#discussion_r974815308
##
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##
@@ -614,7 +615,7 @@ private List getVertexParents(HepRelVertex
vertex) {
private static boolean matchOperands(
RelOptRuleOperand operand,
RelNode rel,
- List bindings,
+ Stack bindings,
Map> nodeChildren) {
if (!operand.matches(rel)) {
return false;
Review Comment:
Could you add a test case?
--
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
[GitHub] [calcite] libenchao closed pull request #2895: [CALCITE-5267] Remove useless variable 'newCasts' in AggregateCaseToF…
libenchao closed pull request #2895: [CALCITE-5267] Remove useless variable 'newCasts' in AggregateCaseToF… URL: https://github.com/apache/calcite/pull/2895 -- 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
[GitHub] [calcite] zabetak commented on a diff in pull request #2911: [CALCITE-5127] Error when executing query with subquery in select lis…
zabetak commented on code in PR #2911:
URL: https://github.com/apache/calcite/pull/2911#discussion_r974202268
##
core/src/main/java/org/apache/calcite/rex/RexUtil.java:
##
@@ -2232,6 +2232,16 @@ public static boolean containsCorrelation(RexNode
condition) {
}
}
+ /** Returns whether an expression contains a {@link RexSubQuery}. */
+ public static boolean containsSubQuery(RexNode node) {
Review Comment:
Maybe change the method to accept as a parameter an `Iterable`
since it will be more convenient for existing callers and it will slightly
reduce duplicate fragments.
##
core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java:
##
@@ -3383,6 +3383,16 @@ void checkCorrelatedMapSubQuery(boolean expand) {
sql(sql).withDecorrelate(true).ok();
}
+ /**
+ * Test case for https://issues.apache.org/jira/browse/CALCITE-5127;>[CALCITE-5127]
+ * Error when executing query with subquery in select list that uses outer
column of array
+ * type.
+ */
+ @Test void testCorrelationWithProjection() {
Review Comment:
Possibly more descriptive names for the test case:
* `testNotMergeProjectionsWhenCorrelationPresent`
* `testProjectionWithCorrelationNoMerge`
##
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##
@@ -1908,9 +1908,19 @@ private RelBuilder project_(
fieldNameList.add(null);
}
+// Do not merge projections when top project contains RexSubQuery
+boolean containsSubQuery = false;
+for (RexNode node : nodes) {
+ if (RexUtil.containsSubQuery(node)) {
+containsSubQuery = true;
+break;
+ }
+}
+
Review Comment:
Also this is a breaking change since it changes the old behavior. It fixes a
bug but at the same time prevents some optimizations to take effect. I think it
is worth adding a few lines explaining the change in behavior in the respective
section in `history.md`.
##
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##
@@ -481,6 +481,18 @@ public TrimResult trimFields(
final int fieldCount = rowType.getFieldCount();
final RelNode input = project.getInput();
+boolean containsSubQuery = false;
+for (RexNode node : project.getProjects()) {
+ if (RexUtil.containsSubQuery(node)) {
+containsSubQuery = true;
+break;
+ }
+}
+// Do not trim Project's fields before SubQueryRemoveRule applies.
+if (containsSubQuery) {
+ return result(project, Mappings.createIdentity(fieldCount));
+}
+
Review Comment:
Does this code has test coverage? I don't see any plan changes due to this.
Also the effects of this change are much broader than the changes in
`RelBuilder` thus I would suggest to treat this separately in another JIRA.
##
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##
@@ -1908,9 +1908,19 @@ private RelBuilder project_(
fieldNameList.add(null);
}
+// Do not merge projections when top project contains RexSubQuery
+boolean containsSubQuery = false;
+for (RexNode node : nodes) {
+ if (RexUtil.containsSubQuery(node)) {
+containsSubQuery = true;
+break;
+ }
+}
+
Review Comment:
I would move the check inside the if block (maybe after line 1934/1944) so
that we don't spend time looking for subqueries if we don't need to.
--
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
[GitHub] [calcite] libenchao commented on pull request #2813: [CALCITE-5127] Error when executing query with subquery in select lis…
libenchao commented on PR #2813: URL: https://github.com/apache/calcite/pull/2813#issuecomment-1250211634 https://github.com/apache/calcite/pull/2911 has been opened, so I'm closing this now. -- 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
[GitHub] [calcite] libenchao closed pull request #2813: [CALCITE-5127] Error when executing query with subquery in select lis…
libenchao closed pull request #2813: [CALCITE-5127] Error when executing query with subquery in select lis… URL: https://github.com/apache/calcite/pull/2813 -- 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
[GitHub] [calcite] libenchao opened a new pull request, #2911: [CALCITE-5127] Error when executing query with subquery in select lis…
libenchao opened a new pull request, #2911: URL: https://github.com/apache/calcite/pull/2911 …t that uses outer column of array type -- 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
[GitHub] [calcite] libenchao closed pull request #2907: [CALCITE-3129] Update site/README.md about how to release the site
libenchao closed pull request #2907: [CALCITE-3129] Update site/README.md about how to release the site URL: https://github.com/apache/calcite/pull/2907 -- 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
[GitHub] [calcite] l4wei opened a new pull request, #2910: [CALCITE-5265] Select operator' parentheses should be same with Union operator
l4wei opened a new pull request, #2910: URL: https://github.com/apache/calcite/pull/2910 remove necessary parentheses on select clause. -- 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
[GitHub] [calcite] birschick-bq commented on pull request #1766: [CALCITE-3745] UnitCompiler can not find required class information.
birschick-bq commented on PR #1766: URL: https://github.com/apache/calcite/pull/1766#issuecomment-1249583288 @Fanoid Just checking in to see if a newer version resolved your issue. -- 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
[GitHub] [calcite] AjinkyaTaranekar commented on pull request #2902: [CALCITE-5275] Release Calcite 1.32.0
AjinkyaTaranekar commented on PR #2902: URL: https://github.com/apache/calcite/pull/2902#issuecomment-1249207194 Hello @julianhyde, SQL [Docs](https://calcite.apache.org/docs/reference.html) are not available at the moment, it says > Not Found > The requested URL was not found on this server. Although if we hit this link [here](https://calcite.apache.org/docs/reference.md), it's available as .md as on [calcite-site](https://github.com/apache/calcite-site/blob/main/docs/reference.md) it's extension in .md instead of .html It got removed at this [commit](https://github.com/apache/calcite-site/commit/feffd77674d8ea555b36ccfb235fead34942d945#diff-1b78c4948b261a285bb8f20b8cd06b6dc9e3a86f0676311984c080a4681fdb86) Could you take a look into this? -- 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
[GitHub] [calcite] Mahesh-Raut closed pull request #2908: Ravtdbq 117 - support for hashrow with multiple arguments
Mahesh-Raut closed pull request #2908: Ravtdbq 117 - support for hashrow with multiple arguments URL: https://github.com/apache/calcite/pull/2908 -- 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
[GitHub] [calcite] libenchao commented on pull request #2907: [CALCITE-3129] Update site/README.md about how to release the site
libenchao commented on PR #2907: URL: https://github.com/apache/calcite/pull/2907#issuecomment-1248118430 @zabetak Thanks for your comment, I merged these two parts now. -- 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
[GitHub] [calcite] viliam-durina commented on pull request #2906: [CALCITE-5285] Preventing CalcReduceExpressionsRule to transform to self
viliam-durina commented on PR #2906: URL: https://github.com/apache/calcite/pull/2906#issuecomment-1246739292 Shouldn't we also add a check that will fail in case `call.transformTo` is supposed to transform to a deep-equal relnode, if this is not allowed in general? -- 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
[GitHub] [calcite] krzysztofslusarski opened a new pull request, #2906: [CALCITE-5285] Preventing CalcReduceExpressionsRule to transform to self
krzysztofslusarski opened a new pull request, #2906: URL: https://github.com/apache/calcite/pull/2906 Fixing https://issues.apache.org/jira/browse/CALCITE-5285 -- 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
[GitHub] [calcite] Aitozi commented on pull request #2901: [CALCITE-5264] Apply hint exclusion check for all rels in the RelOptCall
Aitozi commented on PR #2901: URL: https://github.com/apache/calcite/pull/2901#issuecomment-1242679549 Hi @danny0405 @julianhyde please help review when you are free, thanks -- 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
[GitHub] [calcite] asfgit merged pull request #2903: (do not merge)
asfgit merged PR #2903: URL: https://github.com/apache/calcite/pull/2903 -- 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
[GitHub] [calcite] vvysotskyi closed pull request #2286: [CALCITE-4423] Add method to pass incoming trait in JdbcRules
vvysotskyi closed pull request #2286: [CALCITE-4423] Add method to pass incoming trait in JdbcRules URL: https://github.com/apache/calcite/pull/2286 -- 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
[GitHub] [calcite] aymeric-dispa opened a new pull request, #2904: [CALCITE-5279] Fix asymmetric keyword not supported by Firebolt
aymeric-dispa opened a new pull request, #2904: URL: https://github.com/apache/calcite/pull/2904 Jira ticket: https://issues.apache.org/jira/browse/CALCITE-5279 -- 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
[GitHub] [calcite] asfgit closed pull request #2893: [CALCITE-5262] Add WKB, EWKB, GeoJSON, and GML support
asfgit closed pull request #2893: [CALCITE-5262] Add WKB, EWKB, GeoJSON, and GML support URL: https://github.com/apache/calcite/pull/2893 -- 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
[GitHub] [calcite] asfgit closed pull request #2896: [CALCITE-5270] Make Firebolt dialect return false for supportsAggregateFunctionFilter
asfgit closed pull request #2896: [CALCITE-5270] Make Firebolt dialect return false for supportsAggregateFunctionFilter URL: https://github.com/apache/calcite/pull/2896 -- 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
[GitHub] [calcite] asfgit closed pull request #2878: [CALCITE-5241] Implement CHAR function
asfgit closed pull request #2878: [CALCITE-5241] Implement CHAR function URL: https://github.com/apache/calcite/pull/2878 -- 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
[GitHub] [calcite] rubenada merged pull request #2898: [CALCITE-5274] Improve DocumentBuilderFactory in DiffRepository test class by using secure features
rubenada merged PR #2898: URL: https://github.com/apache/calcite/pull/2898 -- 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
[GitHub] [calcite] rubenada merged pull request #2899: [CALCITE-5277] Make EnumerableRelImplementor stashedParameters order deterministic to increase BINDABLE_CACHE hit rate
rubenada merged PR #2899: URL: https://github.com/apache/calcite/pull/2899 -- 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
[GitHub] [calcite] Aitozi opened a new pull request, #2901: [CALCITE-5264] Apply hint exclusion check for all rels in the RelOptCall
Aitozi opened a new pull request, #2901: URL: https://github.com/apache/calcite/pull/2901 This PR is meant to apply hint exclusion check for all rels in the RelOptCall. This will help to cover the case that the hint exclusion is attatched not on the rel root. -- 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
[GitHub] [calcite] thomasrebele commented on pull request #2899: [CALCITE-5277] Make EnumerableRelImplementor stashedParameters order deterministic to increase BINDABLE_CACHE hit rate
thomasrebele commented on PR #2899: URL: https://github.com/apache/calcite/pull/2899#issuecomment-1240661122 Seems that we have different definitions of simple code ;) I have the impression that an `IdentityHashMap` and an `ArrayList` is a bit easier to understand than a `LinkedHashMap` with a `Equivalence.Wrapper`. A comment at `stashedParameters` could indicate to update the list as well. A check whether the map and the list have the same number of elements could be added. However, this is only a minor detail. If you insist on using the Equivalence class, the PR is fine for me. -- 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
[GitHub] [calcite] dgloeckner opened a new pull request, #2900: CALCITE-5227: add a test case for very wide SELECTs
dgloeckner opened a new pull request, #2900: URL: https://github.com/apache/calcite/pull/2900 This is a test case for https://issues.apache.org/jira/browse/CALCITE-5227 -- 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
[GitHub] [calcite] rubenada commented on pull request #2899: [CALCITE-5277] Make EnumerableRelImplementor stashedParameters order deterministic to increase BINDABLE_CACHE hit rate
rubenada commented on PR #2899: URL: https://github.com/apache/calcite/pull/2899#issuecomment-1240569016 @thomasrebele the code is arguably simpler. There is no need to maintain two different data structures (a map plus a list), just a single map fulfills all our needs. IMO this is less error-prone, in the future someone may need to modify the map, and might forget about updating the list (or vice-versa). It is true that the memory usage can be higher, but I would not expect this to have a huge impact on the overall performance. -- 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
[GitHub] [calcite] thomasrebele commented on pull request #2899: [CALCITE-5277] Make EnumerableRelImplementor stashedParameters order deterministic to increase BINDABLE_CACHE hit rate
thomasrebele commented on PR #2899: URL: https://github.com/apache/calcite/pull/2899#issuecomment-1240487900 Thanks for the PR! What's the advantage of using a `LinkedHashMap` and `Equivalence.Identity` compared to a simple `ArrayList` containing the `ParameterExpression`s in the order they were added? The latter might consume less memory. -- 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
[GitHub] [calcite] rubenada opened a new pull request, #2899: [CALCITE-5277] Make EnumerableRelImplementor stashedParameters order deterministic to increase BINDABLE_CACHE hit rate
rubenada opened a new pull request, #2899: URL: https://github.com/apache/calcite/pull/2899 More info: [CALCITE-5277](https://issues.apache.org/jira/browse/CALCITE-5277) -- 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
[GitHub] [calcite] rubenada opened a new pull request, #2898: [CALCITE-5274] Improve DocumentBuilderFactory in DiffRepository test class by using secure features
rubenada opened a new pull request, #2898: URL: https://github.com/apache/calcite/pull/2898 [[CALCITE-5274](https://issues.apache.org/jira/browse/CALCITE-5274)] Improve DocumentBuilderFactory in DiffRepository test class by using secure features -- 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
[GitHub] [calcite] julianhyde merged pull request #2888: [CALCITE-5251] Support SQL hint for Snapshot
julianhyde merged PR #2888: URL: https://github.com/apache/calcite/pull/2888 -- 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
[GitHub] [calcite] rubenada merged pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
rubenada merged PR #2892: URL: https://github.com/apache/calcite/pull/2892 -- 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
[GitHub] [calcite] rubenada commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
rubenada commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r965255752
##
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##
@@ -60,13 +67,41 @@
public class XmlFunctions {
private static final ThreadLocal<@Nullable XPathFactory> XPATH_FACTORY =
- ThreadLocal.withInitial(XPathFactory::newInstance);
+ ThreadLocal.withInitial(() -> {
+final XPathFactory xPathFactory = XPathFactory.newInstance();
+try {
+ xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
true);
+} catch (XPathFactoryConfigurationException e) {
+ throw new IllegalStateException("XPath Factory configuration
failed", e);
+}
+return xPathFactory;
+ });
private static final ThreadLocal<@Nullable TransformerFactory>
TRANSFORMER_FACTORY =
ThreadLocal.withInitial(() -> {
-TransformerFactory transformerFactory =
TransformerFactory.newInstance();
+final TransformerFactory transformerFactory =
TransformerFactory.newInstance();
transformerFactory.setErrorListener(new InternalErrorListener());
+try {
+
transformerFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+} catch (TransformerConfigurationException e) {
+ throw new IllegalStateException("Transformer Factory configuration
failed", e);
+}
return transformerFactory;
});
+ private static final ThreadLocal<@Nullable DocumentBuilderFactory>
DOCUMENT_BUILDER_FACTORY =
Review Comment:
Thanks @pjfanning , I will take care of it.
--
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
[GitHub] [calcite] pjfanning commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
pjfanning commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r965221384
##
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##
@@ -60,13 +67,41 @@
public class XmlFunctions {
private static final ThreadLocal<@Nullable XPathFactory> XPATH_FACTORY =
- ThreadLocal.withInitial(XPathFactory::newInstance);
+ ThreadLocal.withInitial(() -> {
+final XPathFactory xPathFactory = XPathFactory.newInstance();
+try {
+ xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
true);
+} catch (XPathFactoryConfigurationException e) {
+ throw new IllegalStateException("XPath Factory configuration
failed", e);
+}
+return xPathFactory;
+ });
private static final ThreadLocal<@Nullable TransformerFactory>
TRANSFORMER_FACTORY =
ThreadLocal.withInitial(() -> {
-TransformerFactory transformerFactory =
TransformerFactory.newInstance();
+final TransformerFactory transformerFactory =
TransformerFactory.newInstance();
transformerFactory.setErrorListener(new InternalErrorListener());
+try {
+
transformerFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+} catch (TransformerConfigurationException e) {
+ throw new IllegalStateException("Transformer Factory configuration
failed", e);
+}
return transformerFactory;
});
+ private static final ThreadLocal<@Nullable DocumentBuilderFactory>
DOCUMENT_BUILDER_FACTORY =
Review Comment:
I raised https://issues.apache.org/jira/browse/CALCITE-5274 just in case -
thanks
--
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
[GitHub] [calcite] tjbanghart commented on pull request #2896: [CALCITE-5270] Make Firebolt dialect return false for supportsAggregateFunctionFilter
tjbanghart commented on PR #2896: URL: https://github.com/apache/calcite/pull/2896#issuecomment-1239596312 Thanks for taking a look. I added a simple unit test in `RelToSqlConverterTest`. I think that should be sufficient but please let me know if it needs more. -- 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
[GitHub] [calcite] bchapuis commented on pull request #2893: [CALCITE-5262] Add WKB, EWKB, GeoJSON, and GML support
bchapuis commented on PR #2893: URL: https://github.com/apache/calcite/pull/2893#issuecomment-1239445331 In addition to the support for WKB, EWKB, GeoJSON, and GML, this PR adds support for a lot of the simplest ST_ functions. Most of the test cases have been adapted from examples found in the H2GIS and Postgis documentation. -- 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
[GitHub] [calcite-avatica] Privitar closed pull request #180: Dpp 6545 performance
Privitar closed pull request #180: Dpp 6545 performance URL: https://github.com/apache/calcite-avatica/pull/180 -- 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
[GitHub] [calcite] zabetak commented on pull request #2896: [CALCITE-5270] Make Firebolt dialect return false for supportsAggregateFunctionFilter
zabetak commented on PR #2896: URL: https://github.com/apache/calcite/pull/2896#issuecomment-1239232332 @tjbanghart The best place to add tests about the change is `RelToSqlConverterTest`. Please verify that generated queries actually run in `Firebolt`. -- 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
[GitHub] [calcite] rubenada commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
rubenada commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r964484381
##
core/src/test/java/org/apache/calcite/test/SqlXmlFunctionsTest.java:
##
@@ -36,6 +39,23 @@
*/
class SqlXmlFunctionsTest {
+ private static final String XML = "string";
+ private static final String XSLT =
+ "http://www.w3.org/1999/XSL/Transform\;>";
+ private static final String DOCUMENT_PATH = "/document";
+ private static @Nullable String xmlExternalEntity = null;
+ private static @Nullable String xsltExternalEntity = null;
+
+ @BeforeAll public static void setup() throws Exception {
+final File testFile = File.createTempFile("foo", "temp");
Review Comment:
Modification applied.
Let me know if everything is ok on your side @exceptionfactory . If so, I
will proceed to finalize the PR (squash commits into a single one + merge).
--
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
[GitHub] [calcite] rubenada commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
rubenada commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r964468020
##
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##
@@ -60,13 +67,41 @@
public class XmlFunctions {
private static final ThreadLocal<@Nullable XPathFactory> XPATH_FACTORY =
- ThreadLocal.withInitial(XPathFactory::newInstance);
+ ThreadLocal.withInitial(() -> {
+final XPathFactory xPathFactory = XPathFactory.newInstance();
+try {
+ xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
true);
+} catch (XPathFactoryConfigurationException e) {
+ throw new IllegalStateException("XPath Factory configuration
failed", e);
+}
+return xPathFactory;
+ });
private static final ThreadLocal<@Nullable TransformerFactory>
TRANSFORMER_FACTORY =
ThreadLocal.withInitial(() -> {
-TransformerFactory transformerFactory =
TransformerFactory.newInstance();
+final TransformerFactory transformerFactory =
TransformerFactory.newInstance();
transformerFactory.setErrorListener(new InternalErrorListener());
+try {
+
transformerFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+} catch (TransformerConfigurationException e) {
+ throw new IllegalStateException("Transformer Factory configuration
failed", e);
+}
return transformerFactory;
});
+ private static final ThreadLocal<@Nullable DocumentBuilderFactory>
DOCUMENT_BUILDER_FACTORY =
Review Comment:
Thanks for the comment @pjfanning . The scope of this PR is just
XmlFunctions, but I will create a separate ticket + PR to carry out your
suggestion.
--
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
[GitHub] [calcite] JiajunBernoulli commented on pull request #2896: [CALCITE-5270] Make Firebolt dialect return false for supportsAggregateFunctionFilter
JiajunBernoulli commented on PR #2896: URL: https://github.com/apache/calcite/pull/2896#issuecomment-1238822070 @tjbanghart , would you please add some unit tests? -- 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
[GitHub] [calcite] pjfanning commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
pjfanning commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r964299383
##
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##
@@ -60,13 +67,41 @@
public class XmlFunctions {
private static final ThreadLocal<@Nullable XPathFactory> XPATH_FACTORY =
- ThreadLocal.withInitial(XPathFactory::newInstance);
+ ThreadLocal.withInitial(() -> {
+final XPathFactory xPathFactory = XPathFactory.newInstance();
+try {
+ xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
true);
+} catch (XPathFactoryConfigurationException e) {
+ throw new IllegalStateException("XPath Factory configuration
failed", e);
+}
+return xPathFactory;
+ });
private static final ThreadLocal<@Nullable TransformerFactory>
TRANSFORMER_FACTORY =
ThreadLocal.withInitial(() -> {
-TransformerFactory transformerFactory =
TransformerFactory.newInstance();
+final TransformerFactory transformerFactory =
TransformerFactory.newInstance();
transformerFactory.setErrorListener(new InternalErrorListener());
+try {
+
transformerFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+} catch (TransformerConfigurationException e) {
+ throw new IllegalStateException("Transformer Factory configuration
failed", e);
+}
return transformerFactory;
});
+ private static final ThreadLocal<@Nullable DocumentBuilderFactory>
DOCUMENT_BUILDER_FACTORY =
Review Comment:
Would it be possible to apply similar secure features to the
DocumentBuilderFactory used in
https://github.com/apache/calcite/blob/b9c2099ea92a575084b55a206efc5dd341c0df62/testkit/src/main/java/org/apache/calcite/test/DiffRepository.java#L210
?
--
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
[GitHub] [calcite] tjbanghart opened a new pull request, #2896: [CALCITE-5270] Make Firebolt dialect return false for supportsAggregateFunctionFilter
tjbanghart opened a new pull request, #2896: URL: https://github.com/apache/calcite/pull/2896 Firebolt does not support filter clauses in aggregate functions. e.g. COUNT(*) FILTER (WHERE a = 2). The [dialect implementation](https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/dialect/FireboltSqlDialect.java) should override the `supportsAggregateFunctionFilter` method it currently inherits from the base [SqlDialect class](https://github.com/apache/calcite/blob/b9c2099ea92a575084b55a206efc5dd341c0df62/core/src/main/java/org/apache/calcite/sql/SqlDialect.java#L758) Firebolt SQL reference: https://docs.firebolt.io/sql-reference/functions-reference/ -- 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
[GitHub] [calcite] exceptionfactory commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
exceptionfactory commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r964108841
##
core/src/test/java/org/apache/calcite/test/SqlXmlFunctionsTest.java:
##
@@ -36,6 +39,23 @@
*/
class SqlXmlFunctionsTest {
+ private static final String XML = "string";
+ private static final String XSLT =
+ "http://www.w3.org/1999/XSL/Transform\;>";
+ private static final String DOCUMENT_PATH = "/document";
+ private static @Nullable String xmlExternalEntity = null;
+ private static @Nullable String xsltExternalEntity = null;
+
+ @BeforeAll public static void setup() throws Exception {
+final File testFile = File.createTempFile("foo", "temp");
Review Comment:
Although this is a temporary test file, the documentation for
[java.io.File.createTempFile()](https://docs.oracle.com/javase/7/docs/api/java/io/File.html#createTempFile(java.lang.String,%20java.lang.String))
notes that the
[java.nio.Files.createTempFile()](https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#createTempFile-java.nio.file.Path-java.lang.String-java.lang.String-java.nio.file.attribute.FileAttribute...-)
method creates the file with more restrictive permissions.
--
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
[GitHub] [calcite] zhuwenzhuang commented on pull request #2895: [CALCITE-5267] Remove useless variable 'newCasts' in AggregateCaseToF…
zhuwenzhuang commented on PR #2895: URL: https://github.com/apache/calcite/pull/2895#issuecomment-1238273162 > I checked the history. The rule is introduced from Druid[1], in which newCasts is used. It may be better to correct it instead of removing it. > > [1] https://github.com/apache/druid/pull/4360/files#diff-809e3a73d1485dad83629598ea5bcbab8f5ce7724e9572975b052176c44fbdf7 I guess the code "relBuilder.aggregate(groupKey, newCalls).convert(aggregate.getRowType(), false)" is a better way to do CAST. So we don't need to save every project to keep types stable. -- 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
[GitHub] [calcite] zabetak commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
zabetak commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r963555806
##
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##
@@ -60,11 +67,24 @@
public class XmlFunctions {
private static final ThreadLocal<@Nullable XPathFactory> XPATH_FACTORY =
- ThreadLocal.withInitial(XPathFactory::newInstance);
+ ThreadLocal.withInitial(() -> {
+final XPathFactory xPathFactory = XPathFactory.newInstance();
+try {
+ xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
true);
+} catch (XPathFactoryConfigurationException e) {
+ throw new IllegalStateException("XPath Factory configuration
failed", e);
Review Comment:
Sounds good let's leave it like that and re-evaluate in the future if
necessary.
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #2813: [CALCITE-5127] Error when executing query with subquery in select lis…
zabetak commented on code in PR #2813:
URL: https://github.com/apache/calcite/pull/2813#discussion_r963398493
##
core/src/main/java/org/apache/calcite/rel/core/Project.java:
##
@@ -137,20 +163,27 @@ protected Project(RelInput input) {
* @param input Input
* @param projects Project expressions
* @param rowType Output row type
+ * @param variableSet The variable set.
* @return New {@code Project} if any parameter differs from the value of
this
* {@code Project}, or just {@code this} if all the parameters are
* the same
*
* @see #copy(RelTraitSet, List)
*/
public abstract Project copy(RelTraitSet traitSet, RelNode input,
- List projects, RelDataType rowType);
+ List projects, RelDataType rowType, Set
variableSet);
+
+ @Deprecated // to be removed before 2.0
+ public Project copy(RelTraitSet traitSet, RelNode input,
+ List projects, RelDataType rowType) {
+return copy(traitSet, input, projects, rowType, ImmutableSet.of());
+ }
Review Comment:
At the moment we are not using anywhere the new copy method with the extra
argument (i.e., `variableSet`) so maybe we can revert this change for now.
This is a breaking change since everybody extending `Project` will have to
adapt their code in order the project to compile thus it would be better to
avoid it if it is not an absolute must.
If in the future we find out that we need to copy a projection and change
the variables then we can introduce the extra argument.
##
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##
@@ -1817,7 +1817,24 @@ public RelBuilder project(Iterable
nodes,
*/
public RelBuilder project(Iterable nodes,
Iterable fieldNames, boolean force) {
-return project_(nodes, fieldNames, ImmutableList.of(), force);
+return project(nodes, fieldNames, force, ImmutableSet.of());
+ }
+
+ /**
+ * The same with {@link #project(Iterable, Iterable, boolean)}, with
additional
+ * variablesSet param.
+ *
+ * @param nodes Expressions
+ * @param fieldNames Suggested field names
+ * @param force create project even if it is identity
+ * @param variablesSet Correlating variables that are set when reading a row
+ * from the input, and which may be referenced from the
+ * projection expressions
+ */
+ public RelBuilder project(Iterable nodes,
+ Iterable fieldNames, boolean force,
+ Iterable variablesSet) {
+return project_(nodes, fieldNames, ImmutableList.of(), force,
variablesSet);
Review Comment:
I am again skeptical about the decision to introduce `variablesSet` in the
project in particular due to the second point you mention above. I added some
relevant comments under the JIRA case. We can come back to this if what I wrote
does not make sense.
##
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##
@@ -1908,9 +1927,11 @@ private RelBuilder project_(
fieldNameList.add(null);
}
+// Do not merge projection when top projection has correlation variables
bloat:
if (frame.rel instanceof Project
-&& config.bloat() >= 0) {
+&& config.bloat() >= 0
+&& variables.isEmpty()) {
Review Comment:
Based on my latest comments under the JIRA about not adding variablesSet in
`Project`, I was thinking that something like the snippet below could be used
here and similarly in other places.
```java
if (StreamSupport.stream(nodes.spliterator(),
false).anyMatch(RexUtil::containsCorrelation)) {
break bloat;
}
```
--
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
[GitHub] [calcite] zhuwenzhuang opened a new pull request, #2895: [CALCITE-5267] Remove useless variable 'newCasts' in AggregateCaseToF…
zhuwenzhuang opened a new pull request, #2895: URL: https://github.com/apache/calcite/pull/2895 Remove useless variable 'newCasts' in AggregateCaseToFilter. -- 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
[GitHub] [calcite] rubenada commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
rubenada commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r963054866
##
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##
@@ -60,11 +67,24 @@
public class XmlFunctions {
private static final ThreadLocal<@Nullable XPathFactory> XPATH_FACTORY =
- ThreadLocal.withInitial(XPathFactory::newInstance);
+ ThreadLocal.withInitial(() -> {
+final XPathFactory xPathFactory = XPathFactory.newInstance();
+try {
+ xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
true);
+} catch (XPathFactoryConfigurationException e) {
+ throw new IllegalStateException("XPath Factory configuration
failed", e);
Review Comment:
So, if I understand it correctly, we are breaking applications that cannot
create a secure XPathFactory and which require such factory because they are
calling an xml operator. If an application cannot create a secure XPathFactory
but does not make use of xml operators, it will be fine.
If this is the case, I think we should leave the code as it is: if an xml
operation is invoked, it needs to be done via a secure factory, that seems like
the safest approach.
--
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
[GitHub] [calcite] rubenada commented on a diff in pull request #2892: [CALCITE-5263] Improve XmlFunctions by using an XML DocumentBuilder
rubenada commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r963051308
##
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##
@@ -215,6 +238,27 @@ private static String convertNodeToString(Node node)
throws TransformerException
return writer.toString();
}
+ private static Node getDocumentNode(final String xml) {
+final DocumentBuilderFactory documentBuilderFactory =
DocumentBuilderFactory.newInstance();
Review Comment:
Seems reasonable: created ThreadLocal DocumentBuilderFactory
--
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
