gianm commented on a change in pull request #9773:
URL: https://github.com/apache/druid/pull/9773#discussion_r425274781
##########
File path: docs/querying/sql.md
##########
@@ -526,15 +526,19 @@ the way you write the filter.
2. Try to avoid subqueries underneath joins: they affect both performance and
scalability. This includes implicit
subqueries generated by conditions on mismatched types, and implicit
subqueries generated by conditions that use
-expressions to refer to the right-hand side.
+expressions to refer to the right-hand side.
Review comment:
The stray space shouldn't be here.
##########
File path: docs/querying/datasource.md
##########
@@ -268,12 +268,16 @@ perform best if `d.field` is a string.
4. As of Druid {{DRUIDVERSION}}, the join operator must evaluate the condition
for each row. In the future, we expect
to implement both early and deferred condition evaluation, which we expect to
improve performance considerably for
common use cases.
+5. Currently, Druid does not support pushing down predicates (condition and
filter) pass a Join (i.e. into
Review comment:
past a join (spelling)
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -8749,6 +9097,9 @@ public void testCommaJoinLeftFunction() throws Exception
);
}
+ // This SQL currently does not result in an optimum plan.
+ // Unfortunetly, we have disabled pushing down predicates (conditions and
filters) due to https://github.com/apache/druid/pull/9773
+ // Hence, comma join will result in a cross join with filter on outermost
Review comment:
Please update the docs to point out that people should avoid comma joins
for this reason.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidJoinRule.java
##########
@@ -193,9 +193,16 @@ static boolean canHandleCondition(final RexNode condition,
final RelDataType lef
final int numLeftFields = leftRowType.getFieldCount();
for (RexNode subCondition : subConditions) {
- if (subCondition.isA(SqlKind.LITERAL)) {
- // Literals are always OK.
- literalSubConditions.add((RexLiteral) subCondition);
+ if (RexUtil.isLiteral(subCondition, true)) {
+ if (subCondition instanceof RexCall) {
+ // This is CAST(literal) which is always OK.
+ // We know that this is CASE(literal) as it pass the check from
RexUtil.isLiteral
+ RexCall call = (RexCall) subCondition;
+ literalSubConditions.add((RexLiteral) call.operands.get(0));
Review comment:
We have to verify the types of the cast here, because if the underlying
literal and the cast output type are different, then skipping the cast might
change the meaning of the subcondition. Try looking at the type of the RexCall
and comparing it to the type of the underlying RexLiteral. I think some logic
like this would work:
1. If the types are the same, unwrap the cast and use the underlying literal.
2. If the types are not the same, return `Optional.empty()` indicating the
condition is not supported.
Alternatively, actually apply the cast in step (2) instead of rejecting the
condition. I'm not sure how exactly you would do this or whether it is
necessary. If it turns out to be necessary, perhaps check out what
ReduceExpressionsRule does.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/rule/FilterJoinExcludePushToChildRule.java
##########
@@ -0,0 +1,280 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.rule;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Sets;
+import org.apache.calcite.adapter.enumerable.EnumerableConvention;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Filter;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.rel.rules.FilterJoinRule;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexCall;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexUtil;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.tools.RelBuilderFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public abstract class FilterJoinExcludePushToChildRule extends FilterJoinRule
Review comment:
Please add a javadoc here explaining where the code came from, what
modifications you made, and what might need to happen to make this no longer
necessary (so we can use the builtin rule instead).
##########
File path: docs/querying/sql.md
##########
@@ -526,15 +526,19 @@ the way you write the filter.
2. Try to avoid subqueries underneath joins: they affect both performance and
scalability. This includes implicit
subqueries generated by conditions on mismatched types, and implicit
subqueries generated by conditions that use
-expressions to refer to the right-hand side.
+expressions to refer to the right-hand side.
-3. Read through the [Query execution](query-execution.md) page to understand
how various types of native queries
+3. Currently, Druid does not support pushing down predicates (condition and
filter) pass a Join (i.e. into
Review comment:
past a join (spelling)
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidJoinRule.java
##########
@@ -193,9 +193,16 @@ static boolean canHandleCondition(final RexNode condition,
final RelDataType lef
final int numLeftFields = leftRowType.getFieldCount();
for (RexNode subCondition : subConditions) {
- if (subCondition.isA(SqlKind.LITERAL)) {
- // Literals are always OK.
- literalSubConditions.add((RexLiteral) subCondition);
+ if (RexUtil.isLiteral(subCondition, true)) {
+ if (subCondition instanceof RexCall) {
+ // This is CAST(literal) which is always OK.
+ // We know that this is CASE(literal) as it pass the check from
RexUtil.isLiteral
Review comment:
- CAST, not CASE (spelling)
- passed the check (spelling)
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidJoinRule.java
##########
@@ -193,9 +193,16 @@ static boolean canHandleCondition(final RexNode condition,
final RelDataType lef
final int numLeftFields = leftRowType.getFieldCount();
for (RexNode subCondition : subConditions) {
- if (subCondition.isA(SqlKind.LITERAL)) {
- // Literals are always OK.
- literalSubConditions.add((RexLiteral) subCondition);
+ if (RexUtil.isLiteral(subCondition, true)) {
+ if (subCondition instanceof RexCall) {
Review comment:
A bit nicer to do `if (subCondition.isA(SqlKind.CAST))`, since it's more
explicit as to what we're actually looking for.
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -8749,6 +9097,9 @@ public void testCommaJoinLeftFunction() throws Exception
);
}
+ // This SQL currently does not result in an optimum plan.
+ // Unfortunetly, we have disabled pushing down predicates (conditions and
filters) due to https://github.com/apache/druid/pull/9773
Review comment:
Unfortunately (spelling)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
