imply-cheddar commented on code in PR #13576:
URL: https://github.com/apache/druid/pull/13576#discussion_r1080745376
##########
processing/src/main/java/org/apache/druid/segment/UnnestDimensionCursor.java:
##########
@@ -181,7 +181,10 @@ public void inspectRuntimeShape(RuntimeShapeInspector
inspector)
@Override
public Object getObject()
{
- if (indexedIntsForCurrentRow == null) {
+ if (dimSelector.getObject() == null) {
Review Comment:
Why did we switch away from using the stored reference? `getObject()` can
do work, work we've already done, doing the same work multiple times is bad.
It looks to me like you just needed to switch it to be
```
if (indexedIntsForCurrentRow == null || indexedIntsForCurrentRow.size() == 0)
```
And then, you could perhaps make it even simpler if you check the size once
when setting `indexedIntsForCurrentRow` and set to `null` when size is 0.
##########
.idea/misc.xml:
##########
@@ -84,7 +87,10 @@
<resource url="http://maven.apache.org/ASSEMBLY/2.0.0";
location="$PROJECT_DIR$/.idea/xml-schemas/assembly-2.0.0.xsd" />
<resource url="http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd";
location="$PROJECT_DIR$/.idea/xml-schemas/svg11.dtd" />
</component>
- <component name="ProjectRootManager" version="2" languageLevel="JDK_1_8"
default="false" project-jdk-name="1.8" project-jdk-type="JavaSDK">
+ <component name="ProjectRootManager" version="2" languageLevel="JDK_1_8"
project-jdk-name="1.8" project-jdk-type="JavaSDK">
<output url="file://$PROJECT_DIR$/classes" />
</component>
-</project>
+ <component name="SuppressKotlinCodeStyleNotification">
+ <option name="disableForAll" value="true" />
+ </component>
Review Comment:
The changes in this file appear to continue to exist in this PR, calling it
out as it should likely be reverted before a merge.
##########
processing/src/main/java/org/apache/druid/segment/UnnestDimensionCursor.java:
##########
@@ -409,7 +412,12 @@ public int size()
@Override
public int get(int idx)
{
- return indexedIntsForCurrentRow.get(index);
+ // to support rows which have only null values
+ // need to check if the value is not null and the size is greater than 0
+ if (indexedIntsForCurrentRow != null && indexedIntsForCurrentRow.size()
> 0) {
+ return indexedIntsForCurrentRow.get(index);
+ }
Review Comment:
Why would we be getting any rows at all when unnesting a data point that was
null to begin with? That is, if we had the array-of-null, we should be getting
an IndexedInts back with 1 entry, which is null. If we didn't have anything to
begin with, then we should be getting back `null` and probably don't have any
work to do on the row anyway?
##########
processing/src/main/java/org/apache/druid/query/InlineDataSource.java:
##########
@@ -125,7 +125,7 @@ private static boolean rowsEqual(final Iterable<Object[]>
rowsA, final Iterable<
final Object[] rowA = listA.get(i);
final Object[] rowB = listB.get(i);
- if (!Arrays.equals(rowA, rowB)) {
+ if (!Arrays.deepEquals(rowA, rowB)) {
Review Comment:
There's a `hashCode` method that claims that it is compatible with this
method. I'm not sure that is actually true anymore because I don't believe it
walks into Arrays-of-Arrays the same way that this one does.
##########
processing/src/test/java/org/apache/druid/segment/UnnestColumnValueSelectorCursorTest.java:
##########
@@ -592,7 +590,25 @@ public void test_list_unnest_cursors_dimSelector()
OUTPUT_NAME,
IGNORE_SET
);
-
unnestCursor.getColumnSelectorFactory().makeDimensionSelector(DefaultDimensionSpec.of(OUTPUT_NAME));
+ // should return a column value selector for this case
+ BaseSingleValueDimensionSelector unnestDimSelector =
(BaseSingleValueDimensionSelector) unnestCursor.getColumnSelectorFactory()
+
.makeDimensionSelector(
+
DefaultDimensionSpec.of(
+
OUTPUT_NAME));
+ unnestDimSelector.inspectRuntimeShape(null);
+ int k = 0;
+ while (!unnestCursor.isDone()) {
+ if (k < 8) {
+ Assert.assertEquals(unnestDimSelector.getValue(),
expectedResults.get(k).toString());
Review Comment:
I think you have expected and actual inverted. The junit assert has
expected come first.
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -214,12 +217,43 @@ 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
+
+ //use this index to return
+ final int index =
rowSignature.getColumnNames().indexOf(ref.getField().getName());
+ if (ref.getField().getIndex() > rowSignature.size()) {
+ throw new CannotBuildQueryException(StringUtils.format(
+ "Cannot build query as column name [%s] does not exist in row [%s]",
ref.getField().getName(), rowSignature)
+ );
+ }
+
+ final String columnName = rowSignature.getColumnName(index);
Review Comment:
Didn't we just search the rowSignature for the name? Why will we get a
different name back than the one that we searched for?
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -214,12 +217,43 @@ 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
+
+ //use this index to return
+ final int index =
rowSignature.getColumnNames().indexOf(ref.getField().getName());
+ if (ref.getField().getIndex() > rowSignature.size()) {
+ throw new CannotBuildQueryException(StringUtils.format(
+ "Cannot build query as column name [%s] does not exist in row [%s]",
ref.getField().getName(), rowSignature)
+ );
+ }
+
+ final String columnName = rowSignature.getColumnName(index);
+ final Optional<ColumnType> columnType = rowSignature.getColumnType(index);
+ if (columnName == null) {
+ throw new ISE("Expression referred to nonexistent index [%d] in row
[%s]", index, rowSignature);
+ }
Review Comment:
I don't believe this if block can ever be run.
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -214,12 +217,43 @@ 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
Review Comment:
I feel like this comment has been separated from its home. Is it inside of
the if down below?
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -214,12 +217,43 @@ 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
+
+ //use this index to return
+ final int index =
rowSignature.getColumnNames().indexOf(ref.getField().getName());
+ if (ref.getField().getIndex() > rowSignature.size()) {
+ throw new CannotBuildQueryException(StringUtils.format(
+ "Cannot build query as column name [%s] does not exist in row [%s]",
ref.getField().getName(), rowSignature)
+ );
+ }
+
+ final String columnName = rowSignature.getColumnName(index);
+ final Optional<ColumnType> columnType = rowSignature.getColumnType(index);
+ if (columnName == null) {
+ throw new ISE("Expression referred to nonexistent index [%d] in row
[%s]", index, rowSignature);
+ }
+
+ return DruidExpression.ofColumn(columnType.orElse(null), columnName);
Review Comment:
Why would we ever not have the columnType? Why do we need to orElse it?
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidCorrelateUnnestRel.java:
##########
@@ -0,0 +1,300 @@
+/*
+ * 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.RelOptCost;
+import org.apache.calcite.plan.RelOptPlanner;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Correlate;
+import org.apache.calcite.rel.core.Filter;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.metadata.RelMetadataQuery;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+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.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class DruidCorrelateUnnestRel extends DruidRel<DruidCorrelateUnnestRel>
Review Comment:
Please add a clone() method. If we don't think that clone() is actually
called, you can make it throw UnsupportedOperationException.
--
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]
