clintropolis commented on code in PR #13576:
URL: https://github.com/apache/druid/pull/13576#discussion_r1055871100
##########
.idea/misc.xml:
##########
@@ -46,7 +46,7 @@
<option name="myDefaultNotNull" value="javax.annotation.Nonnull" />
<option name="myNullables">
<value>
- <list size="12">
Review Comment:
I assume changes to this file were unintended?
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -214,12 +216,39 @@ public static DruidExpression
toDruidExpressionWithPostAggOperands(
return rexCallToDruidExpression(plannerContext, rowSignature, rexNode,
postAggregatorVisitor);
} else if (kind == SqlKind.LITERAL) {
return literalToDruidExpression(plannerContext, rexNode);
+ } else if (kind == SqlKind.FIELD_ACCESS) {
+ return fieldAccessToDruidExpression(rowSignature, rexNode);
} else {
// Can't translate.
return null;
}
}
+ private static DruidExpression fieldAccessToDruidExpression(
+ final RowSignature rowSignature,
+ final RexNode rexNode
+ )
+ {
+ // Translate field references.
+ final RexFieldAccess ref = (RexFieldAccess) rexNode;
+ // This case arises in the case of a correlation where the rexNode points
to a table from the left subtree
+ // while the underlying datasource is the scan stub created from
LogicalValuesRule
+ // In such a case we throw a CannotBuildQueryException so that Calcite
does not go ahead with this path
+ // This exception is caught while returning false from isValidDruidQuery()
method
+ if (ref.getField().getIndex() > rowSignature.size()) {
+ throw new CannotBuildQueryException(
+ "Cannot build query as index is higher than row size"
+ );
+ }
+ final String columnName =
rowSignature.getColumnName(ref.getField().getIndex());
+ final Optional<ColumnType> columnType =
rowSignature.getColumnType(ref.getField().getIndex());
+ if (columnName == null) {
+ throw new ISE("Expression referred to nonexistent index[%d]",
ref.getField().getIndex());
Review Comment:
same comment about error message
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -214,12 +216,39 @@ public static DruidExpression
toDruidExpressionWithPostAggOperands(
return rexCallToDruidExpression(plannerContext, rowSignature, rexNode,
postAggregatorVisitor);
} else if (kind == SqlKind.LITERAL) {
return literalToDruidExpression(plannerContext, rexNode);
+ } else if (kind == SqlKind.FIELD_ACCESS) {
+ return fieldAccessToDruidExpression(rowSignature, rexNode);
} else {
// Can't translate.
return null;
}
}
+ private static DruidExpression fieldAccessToDruidExpression(
+ final RowSignature rowSignature,
+ final RexNode rexNode
+ )
+ {
+ // Translate field references.
+ final RexFieldAccess ref = (RexFieldAccess) rexNode;
+ // This case arises in the case of a correlation where the rexNode points
to a table from the left subtree
+ // while the underlying datasource is the scan stub created from
LogicalValuesRule
+ // In such a case we throw a CannotBuildQueryException so that Calcite
does not go ahead with this path
+ // This exception is caught while returning false from isValidDruidQuery()
method
+ if (ref.getField().getIndex() > rowSignature.size()) {
+ throw new CannotBuildQueryException(
+ "Cannot build query as index is higher than row size"
Review Comment:
suggest showing column/field name that is missing from row signature instead
of saying anything about index since is confusing
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidCorrelateUnnestRel.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.rel;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.core.Filter;
+import org.apache.calcite.rel.logical.LogicalCorrelate;
+import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.UnnestDataSource;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.Expressions;
+import org.apache.druid.sql.calcite.planner.PlannerConfig;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.table.RowSignatures;
+
+import javax.annotation.Nullable;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class DruidCorrelateUnnestRel extends DruidRel<DruidCorrelateUnnestRel>
+{
+ // This may be needed for the explain plan later
+ // private static final TableDataSource DUMMY_DATA_SOURCE = new
TableDataSource("__unnest__");
Review Comment:
nit: is this needed? else remove
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -372,6 +392,23 @@ private static Projection computeSelectProjection(
}
}
+ @Nonnull
+ private static Projection computeUnnestProjection(
+ final PartialDruidQuery partialQuery,
+ final PlannerContext plannerContext,
+ final RowSignature rowSignature,
+ final VirtualColumnRegistry virtualColumnRegistry
+ )
+ {
+ final Project project =
Preconditions.checkNotNull(partialQuery.getUnnestProject(), "unnestProject");
+
+ if (partialQuery.getAggregate() != null) {
+ throw new ISE("Cannot have both 'unnestProject' and 'aggregate', how can
this be?");
Review Comment:
why not?
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidCorrelateUnnestRel.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.rel;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.core.Filter;
+import org.apache.calcite.rel.logical.LogicalCorrelate;
+import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.UnnestDataSource;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.Expressions;
+import org.apache.druid.sql.calcite.planner.PlannerConfig;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.table.RowSignatures;
+
+import javax.annotation.Nullable;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class DruidCorrelateUnnestRel extends DruidRel<DruidCorrelateUnnestRel>
+{
+ // This may be needed for the explain plan later
+ // private static final TableDataSource DUMMY_DATA_SOURCE = new
TableDataSource("__unnest__");
+ private final PartialDruidQuery partialQuery;
+ private final PlannerConfig plannerConfig;
+ private final LogicalCorrelate logicalCorrelate;
+ private final DataSource baseDataSource;
+ private final DruidQueryRel druidQueryRel;
+ private final Filter baseFilter;
+ private DruidUnnestDatasourceRel unnestDatasourceRel;
+
+ public DruidCorrelateUnnestRel(
+ RelOptCluster cluster,
+ RelTraitSet traitSet,
+ LogicalCorrelate logicalCorrelateRel,
+ PartialDruidQuery partialQuery,
+ DruidQueryRel druidQueryRel,
+ DruidUnnestDatasourceRel unnestDatasourceRel,
+ Filter baseFilter,
+ PlannerContext plannerContext
+ )
+ {
+ super(cluster, traitSet, plannerContext);
+ this.logicalCorrelate = logicalCorrelateRel;
+ this.partialQuery = partialQuery;
+ this.plannerConfig = plannerContext.getPlannerConfig();
+ this.druidQueryRel = druidQueryRel;
+ this.baseDataSource = druidQueryRel.getDruidTable().getDataSource();
+ this.unnestDatasourceRel = unnestDatasourceRel;
+ this.baseFilter = baseFilter;
+ }
+
+
+ @Nullable
+ @Override
+ public PartialDruidQuery getPartialDruidQuery()
+ {
+ return partialQuery;
+ }
+
+ @Override
+ public DruidCorrelateUnnestRel withPartialQuery(PartialDruidQuery
newQueryBuilder)
+ {
+ return new DruidCorrelateUnnestRel(
+ getCluster(),
+ getTraitSet().plusAll(newQueryBuilder.getRelTraits()),
+ logicalCorrelate,
+ newQueryBuilder,
+ druidQueryRel,
+ unnestDatasourceRel,
+ baseFilter,
+ getPlannerContext()
+ );
+ }
+
+ @Override
+ public DruidQuery toDruidQuery(boolean finalizeAggregations)
+ {
+ final RowSignature rowSignature = RowSignatures.fromRelDataType(
+ logicalCorrelate.getRowType().getFieldNames(),
+ logicalCorrelate.getRowType()
+ );
+
+ final DruidQuery leftQuery =
Preconditions.checkNotNull((druidQueryRel).toDruidQuery(false), "leftQuery");
+ final DataSource leftDataSource;
+
+ if (DruidJoinQueryRel.computeLeftRequiresSubquery(druidQueryRel)) {
+ leftDataSource = new QueryDataSource(leftQuery.getQuery());
+ } else {
+ leftDataSource = leftQuery.getDataSource();
+ }
+
+ final DruidExpression expression = Expressions.toDruidExpression(
+ getPlannerContext(),
+ rowSignature,
+ unnestDatasourceRel.getUnnestProject().getProjects().get(0)
+ );
+
+ LogicalProject unnestProject = LogicalProject.create(
+ this,
+ ImmutableList.of(unnestDatasourceRel.getUnnestProject()
+ .getProjects()
+ .get(0)),
+ unnestDatasourceRel.getUnnestProject().getRowType()
+ );
+
+ String dimensionToUnnest;
Review Comment:
nit: final? also suggest using `inputToUnnest` or something since the input
doesn't have to be a dimension
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/PartialDruidQuery.java:
##########
@@ -54,6 +54,8 @@
private final RelNode scan;
private final Filter whereFilter;
private final Project selectProject;
+ // add an unnestProject
Review Comment:
nit: remove comment
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidCorrelateUnnestRule.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+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.Project;
+import org.apache.calcite.rel.logical.LogicalCorrelate;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexUtil;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.rel.DruidCorrelateUnnestRel;
+import org.apache.druid.sql.calcite.rel.DruidQueryRel;
+import org.apache.druid.sql.calcite.rel.DruidRel;
+import org.apache.druid.sql.calcite.rel.DruidUnnestDatasourceRel;
+import org.apache.druid.sql.calcite.rel.PartialDruidQuery;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class DruidCorrelateUnnestRule extends RelOptRule
+{
+ private final PlannerContext plannerContext;
+
+ public DruidCorrelateUnnestRule(final PlannerContext plannerContext)
+ {
+ super(
+ operand(
+ LogicalCorrelate.class,
+ operand(DruidRel.class, any()),
+ operand(DruidUnnestDatasourceRel.class, any())
+ )
+ );
+
+ this.plannerContext = plannerContext;
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call)
+ {
+ LogicalCorrelate logicalCorrelate = call.rel(0);
+ DruidQueryRel druidQueryRel = call.rel(1);
+ DruidUnnestDatasourceRel unnestDatasourceRel = call.rel(2);
+ final Filter leftFilter;
+ final DruidRel<?> newLeft;
+ final RexBuilder rexBuilder =
logicalCorrelate.getCluster().getRexBuilder();
+ final List<RexNode> newProjectExprs = new ArrayList<>();
+
+ if (druidQueryRel.getPartialDruidQuery().stage() ==
PartialDruidQuery.Stage.SELECT_PROJECT) {
+ // Swap the left-side projection above the correlate, so the left side
is a simple scan or mapping. This helps us
+ // avoid subqueries.
+ final RelNode leftScan = druidQueryRel.getPartialDruidQuery().getScan();
+ final Project leftProject =
druidQueryRel.getPartialDruidQuery().getSelectProject();
+ leftFilter = druidQueryRel.getPartialDruidQuery().getWhereFilter();
+
+ // Left-side projection expressions rewritten to be on top of the
correlate.
+ newProjectExprs.addAll(leftProject.getProjects());
+ newLeft =
druidQueryRel.withPartialQuery(PartialDruidQuery.create(leftScan));
+ } else {
+ // Leave left as-is. Write input refs that do nothing.
+ for (int i = 0; i < druidQueryRel.getRowType().getFieldCount(); i++) {
+
newProjectExprs.add(rexBuilder.makeInputRef(logicalCorrelate.getRowType().getFieldList().get(i).getType(),
i));
+ }
+ newLeft = druidQueryRel;
+ leftFilter = null;
+ }
+
+
+ if (unnestDatasourceRel.getPartialDruidQuery().stage() ==
PartialDruidQuery.Stage.SELECT_PROJECT) {
+ for (final RexNode rexNode : RexUtil.shift(
+ unnestDatasourceRel.getPartialDruidQuery()
+ .getSelectProject()
+ .getProjects(),
+ newLeft.getRowType().getFieldCount()
+ )) {
+ newProjectExprs.add(rexNode);
+ }
+ } else {
+ // Leave right as-is. Write input refs that do nothing.
+ for (int i = 0; i < unnestDatasourceRel.getRowType().getFieldCount();
i++) {
+ newProjectExprs.add(
+ rexBuilder.makeInputRef(
+ logicalCorrelate.getRowType()
+ .getFieldList()
+
.get(druidQueryRel.getRowType().getFieldCount() + i)
+ .getType(),
+ newLeft.getRowType().getFieldCount() + i
+ )
+ );
+ }
+ }
+
+
+ // todo: make new projects with druidQueryRel projects + unnestRel
projects shifted
Review Comment:
nit: this todo seems 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
