You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2023/03/07 12:23:24 UTC
[GitHub] [druid] gianm opened a new pull request, #13892: Various changes and fixes to UNNEST.
gianm opened a new pull request, #13892:
URL: https://github.com/apache/druid/pull/13892
Native changes:
1) UnnestDataSource: Replace "column" and "outputName" with "virtualColumn".
This enables pushing expressions into the datasource. This in turn
allows us to do the next thing...
2) UnnestStorageAdapter: Logically apply query-level filters and virtual
columns after the unnest operation. (Physically, filters are pulled up,
when possible.) This is beneficial because it allows filters and
virtual columns to reference the unnested column, and because it is
consistent with how the join datasource works.
3) Various documentation updates, including declaring "unnest" as an
experimental feature for now.
SQL changes:
1) Rename DruidUnnestRel (& Rule) to DruidUnnestRel (& Rule). The rel
is simplified: it only handles the UNNEST part of a correlated join.
Constant UNNESTs are handled with regular inline rels.
2) Rework DruidCorrelateUnnestRule to focus on pulling Projects from
the left side up above the Correlate. New test testUnnestTwice verifies
that this works even when two UNNESTs are stacked on the same table.
3) Include ProjectCorrelateTransposeRule from Calcite to encourage
pushing mappings down below the left-hand side of the Correlate.
4) Add a new CorrelateFilterLTransposeRule and CorrelateFilterRTransposeRule
to handle pulling Filters up above the Correlate. New tests
testUnnestWithFiltersOutside and testUnnestTwiceWithFilters verify
this behavior.
5) Require a context feature flag for SQL UNNEST, since it's undocumented.
As part of this, also cleaned up how we handle feature flags in SQL.
They're now hooked into EngineFeatures, which is useful because not
all engines support all 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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1132106703
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -226,9 +247,146 @@ public Metadata getMetadata()
return baseAdapter.getMetadata();
}
- public String getDimensionToUnnest()
+ public VirtualColumn getUnnestColumn()
{
- return dimensionToUnnest;
+ return unnestColumn;
}
-}
+ /**
+ * Split queryFilter into pre- and post-correlate filters.
+ *
+ * @param queryFilter query filter passed to makeCursors
+ * @param queryVirtualColumns query virtual columns passed to makeCursors
+ * @param inputColumn input column to unnest if it's a direct access; otherwise null
+ * @param inputColumnCapabilites input column capabilities if known; otherwise null
+ *
+ * @return pair of pre- and post-correlate filters
+ */
+ private Pair<Filter, Filter> computeBaseAndPostCorrelateFilters(
+ @Nullable final Filter queryFilter,
+ final VirtualColumns queryVirtualColumns,
+ @Nullable final String inputColumn,
+ @Nullable final ColumnCapabilities inputColumnCapabilites
+ )
+ {
+ class FilterSplitter
+ {
+ final List<Filter> preFilters = new ArrayList<>();
+ final List<Filter> postFilters = new ArrayList<>();
+
+ void add(@Nullable final Filter filter)
+ {
+ if (filter == null) {
+ return;
+ }
+
+ final Set<String> requiredColumns = filter.getRequiredColumns();
+
+ // Run filter post-correlate if it refers to any virtual columns.
+ if (queryVirtualColumns.getVirtualColumns().length > 0) {
+ for (String column : requiredColumns) {
+ if (queryVirtualColumns.exists(column)) {
+ postFilters.add(filter);
+ return;
+ }
+ }
+ }
+
+ if (requiredColumns.contains(outputColumnName)) {
+ // Try to move filter pre-correlate if possible.
+ final Filter newFilter = rewriteFilterOnUnnestColumnIfPossible(filter, inputColumn, inputColumnCapabilites);
+ if (newFilter != null) {
+ preFilters.add(newFilter);
Review Comment:
Here's the patch https://gist.github.com/gianm/86d55fb6e5ee58c935152761a9df4a4d. Feel free to roll this into what you're doing @somu-imply.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1132583061
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -226,9 +247,146 @@ public Metadata getMetadata()
return baseAdapter.getMetadata();
}
- public String getDimensionToUnnest()
+ public VirtualColumn getUnnestColumn()
{
- return dimensionToUnnest;
+ return unnestColumn;
}
-}
+ /**
+ * Split queryFilter into pre- and post-correlate filters.
+ *
+ * @param queryFilter query filter passed to makeCursors
+ * @param queryVirtualColumns query virtual columns passed to makeCursors
+ * @param inputColumn input column to unnest if it's a direct access; otherwise null
+ * @param inputColumnCapabilites input column capabilities if known; otherwise null
+ *
+ * @return pair of pre- and post-correlate filters
+ */
+ private Pair<Filter, Filter> computeBaseAndPostCorrelateFilters(
+ @Nullable final Filter queryFilter,
+ final VirtualColumns queryVirtualColumns,
+ @Nullable final String inputColumn,
+ @Nullable final ColumnCapabilities inputColumnCapabilites
+ )
+ {
+ class FilterSplitter
+ {
+ final List<Filter> preFilters = new ArrayList<>();
+ final List<Filter> postFilters = new ArrayList<>();
+
+ void add(@Nullable final Filter filter)
+ {
+ if (filter == null) {
+ return;
+ }
+
+ final Set<String> requiredColumns = filter.getRequiredColumns();
+
+ // Run filter post-correlate if it refers to any virtual columns.
+ if (queryVirtualColumns.getVirtualColumns().length > 0) {
+ for (String column : requiredColumns) {
+ if (queryVirtualColumns.exists(column)) {
+ postFilters.add(filter);
+ return;
+ }
+ }
+ }
+
+ if (requiredColumns.contains(outputColumnName)) {
+ // Try to move filter pre-correlate if possible.
+ final Filter newFilter = rewriteFilterOnUnnestColumnIfPossible(filter, inputColumn, inputColumnCapabilites);
+ if (newFilter != null) {
+ preFilters.add(newFilter);
Review Comment:
Thanks, I'll add this up
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 merged pull request #13892: Various changes and fixes to UNNEST.
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 merged PR #13892:
URL: https://github.com/apache/druid/pull/13892
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1128559650
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -122,12 +121,16 @@ public Sequence<Cursor> makeCursors(
retVal = new UnnestColumnValueSelectorCursor(
retVal,
retVal.getColumnSelectorFactory(),
- dimensionToUnnest,
+ unnestColumn,
outputColumnName,
allowSet
);
}
- return retVal;
+ return PostJoinCursor.wrap(
Review Comment:
This is nice, there's no need to pass in the allowFilter inside the data source now and the PostJoinCursor will apply the filters on unnested column which are the post join filters
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on PR #13892:
URL: https://github.com/apache/druid/pull/13892#issuecomment-1459002744
I think the reason is that there is an additional level of LogicalProject between Correlate and LogicalFilter in the query where `SELECT d3 FROM (select * from druid.numfoo where dim2='a'), UNNEST(MV_TO_ARRAY(dim3)) as unnested (d3)` it plans to
```
136:LogicalProject(d3=[$17])
134:LogicalCorrelate(subset=[rel#135:Subset#7.NONE.[]], correlation=[$cor0], joinType=[inner], requiredColumns=[{3}])
125:LogicalProject(subset=[rel#126:Subset#2.NONE.[]], __time=[$0], dim1=[$1], dim2=[CAST('a':VARCHAR):VARCHAR], dim3=[$3], dim4=[$4], dim5=[$5], dim6=[$6], d1=[$7], d2=[$8], f1=[$9], f2=[$10], l1=[$11], l2=[$12], cnt=[$13], m1=[$14], m2=[$15], unique_dim1=[$16])
123:LogicalFilter(subset=[rel#124:Subset#1.NONE.[]], condition=[=($2, 'a')])
9:LogicalTableScan(subset=[rel#122:Subset#0.NONE.[]], table=[[druid, numfoo]])
130:Uncollect(subset=[rel#131:Subset#5.NONE.[]])
128:LogicalProject(subset=[rel#129:Subset#4.NONE.[]], EXPR$0=[MV_TO_ARRAY($cor0.dim3)])
12:LogicalValues(subset=[rel#127:Subset#3.NONE.[0]], tuples=[[{ 0 }]])
```
which does not match the CorrelateFilterTranspose rule (L or R). We might need to address that but can be done with a later PR as well
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #13892: Various changes and fixes to UNNEST.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #13892:
URL: https://github.com/apache/druid/pull/13892#issuecomment-1461826876
> But these 2 queries have different plans, the selector filter inside the table for the first query below plans as an unnest over a QueryDataSource while the second one is an Unnest over a TableDataSource
Looks like you're right about the extra Project being the reason. It prevents the filter from being pulled out since it's in the way. One approach we could take here is make a rule that recognizes the situation, and pulls the Project and Filter both out.
Thanks for taking a look. I pushed up fixes to the merge conflicts and the one test you pointed out. I also simplified DruidUnnestRel— in the latest patch it only contains a single RexNode. Finally I also expanded the filter-rewriting logic in UnnestStorageAdapter so it rewrites certain filters to use the input column of the unnest rather than the output column. It's limited to string types and non-expression filters, but it's s start. This piece could be improved in the future.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1131566874
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -226,9 +247,146 @@ public Metadata getMetadata()
return baseAdapter.getMetadata();
}
- public String getDimensionToUnnest()
+ public VirtualColumn getUnnestColumn()
{
- return dimensionToUnnest;
+ return unnestColumn;
}
-}
+ /**
+ * Split queryFilter into pre- and post-correlate filters.
+ *
+ * @param queryFilter query filter passed to makeCursors
+ * @param queryVirtualColumns query virtual columns passed to makeCursors
+ * @param inputColumn input column to unnest if it's a direct access; otherwise null
+ * @param inputColumnCapabilites input column capabilities if known; otherwise null
+ *
+ * @return pair of pre- and post-correlate filters
+ */
+ private Pair<Filter, Filter> computeBaseAndPostCorrelateFilters(
+ @Nullable final Filter queryFilter,
+ final VirtualColumns queryVirtualColumns,
+ @Nullable final String inputColumn,
+ @Nullable final ColumnCapabilities inputColumnCapabilites
+ )
+ {
+ class FilterSplitter
+ {
+ final List<Filter> preFilters = new ArrayList<>();
+ final List<Filter> postFilters = new ArrayList<>();
+
+ void add(@Nullable final Filter filter)
+ {
+ if (filter == null) {
+ return;
+ }
+
+ final Set<String> requiredColumns = filter.getRequiredColumns();
+
+ // Run filter post-correlate if it refers to any virtual columns.
+ if (queryVirtualColumns.getVirtualColumns().length > 0) {
+ for (String column : requiredColumns) {
+ if (queryVirtualColumns.exists(column)) {
+ postFilters.add(filter);
+ return;
+ }
+ }
+ }
+
+ if (requiredColumns.contains(outputColumnName)) {
+ // Try to move filter pre-correlate if possible.
+ final Filter newFilter = rewriteFilterOnUnnestColumnIfPossible(filter, inputColumn, inputColumnCapabilites);
+ if (newFilter != null) {
+ preFilters.add(newFilter);
Review Comment:
Let's merge this once CI passes, I'll fix them up. I understand the case where it returns the full row even for a match in MVDs. I'll also need to remove the allowSet and minor other changes. Will followup as soon as this gets merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1129981804
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java:
##########
@@ -2732,6 +2744,139 @@ public void testUnnest()
);
}
+ @Test
+ public void testUnnestTwice()
+ {
+ cannotVectorize();
+ testQuery(
+ "SELECT dim1, MV_TO_ARRAY(dim3), STRING_TO_ARRAY(dim1, U&'\\005C.') AS dim1_split, dim1_split_unnest, dim3_unnest\n"
+ + "FROM\n"
+ + " druid.numfoo,\n"
+ + " UNNEST(STRING_TO_ARRAY(dim1, U&'\\005C.')) as t2 (dim1_split_unnest),\n"
+ + " UNNEST(MV_TO_ARRAY(dim3)) as t3 (dim3_unnest)",
+ QUERY_CONTEXT_UNNEST,
+ ImmutableList.of(
+ Druids.newScanQueryBuilder()
+ .dataSource(
+ UnnestDataSource.create(
+ UnnestDataSource.create(
+ new TableDataSource(CalciteTests.DATASOURCE3),
+ expressionVirtualColumn(
+ "j0.unnest",
+ "string_to_array(\"dim1\",'\\u005C.')",
+ ColumnType.STRING_ARRAY
+ ),
+ null
+ ),
+ expressionVirtualColumn(
+ "_j0.unnest",
+ "\"dim3\"",
+ ColumnType.STRING
+ ),
+ null
+ )
+ )
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn(
+ "v0",
+ "mv_to_array(\"dim3\")",
+ ColumnType.STRING_ARRAY
+ ),
+ expressionVirtualColumn(
+ "v1",
+ "string_to_array(\"dim1\",'\\u005C.')",
+ ColumnType.STRING_ARRAY
+ )
+ )
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .legacy(false)
+ .context(QUERY_CONTEXT_UNNEST)
+ .columns(ImmutableList.of("_j0.unnest", "dim1", "j0.unnest", "v0", "v1"))
+ .build()
+ ),
+ ImmutableList.of(
+ new Object[]{"", ImmutableList.of("a", "b"), useDefault ? null : ImmutableList.of(""), "", "a"},
+ new Object[]{"", ImmutableList.of("a", "b"), useDefault ? null : ImmutableList.of(""), "", "b"},
+ new Object[]{"10.1", ImmutableList.of("b", "c"), ImmutableList.of("10", "1"), "10", "b"},
+ new Object[]{"10.1", ImmutableList.of("b", "c"), ImmutableList.of("10", "1"), "10", "c"},
+ new Object[]{"10.1", ImmutableList.of("b", "c"), ImmutableList.of("10", "1"), "1", "b"},
+ new Object[]{"10.1", ImmutableList.of("b", "c"), ImmutableList.of("10", "1"), "1", "c"},
+ new Object[]{"2", ImmutableList.of("d"), ImmutableList.of("2"), "2", "d"},
+ new Object[]{"1", useDefault ? null : ImmutableList.of(""), ImmutableList.of("1"), "1", ""},
+ new Object[]{"def", null, ImmutableList.of("def"), "def", NullHandling.defaultStringValue()},
+ new Object[]{"abc", null, ImmutableList.of("abc"), "abc", NullHandling.defaultStringValue()}
+ )
+ );
+ }
+
+ @Test
+ public void testUnnestTwiceWithFiltersAndExpressions()
+ {
+ cannotVectorize();
+ testQuery(
+ "SELECT dim1, MV_TO_ARRAY(dim3), STRING_TO_ARRAY(dim1, U&'\\005C.') AS dim1_split, dim1_split_unnest, dim3_unnest || 'xx' AS dim3_unnest\n"
+ + "FROM\n"
+ + " druid.numfoo,\n"
+ + " UNNEST(STRING_TO_ARRAY(dim1, U&'\\005C.')) as t2 (dim1_split_unnest),\n"
+ + " UNNEST(MV_TO_ARRAY(dim3)) as t3 (dim3_unnest)"
+ + "WHERE t2.dim1_split_unnest IN ('1', '2')",
+ QUERY_CONTEXT_UNNEST,
+ ImmutableList.of(
+ Druids.newScanQueryBuilder()
+ .dataSource(
+ UnnestDataSource.create(
+ UnnestDataSource.create(
+ new TableDataSource(CalciteTests.DATASOURCE3),
+ expressionVirtualColumn(
+ "j0.unnest",
+ "string_to_array(\"dim1\",'\\u005C.')",
+ ColumnType.STRING_ARRAY
+ ),
+ null
+ ),
+ expressionVirtualColumn(
+ "_j0.unnest",
+ "\"dim3\"",
+ ColumnType.STRING
+ ),
+ null
+ )
+ )
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn(
+ "v0",
+ "mv_to_array(\"dim3\")",
+ ColumnType.STRING_ARRAY
+ ),
+ expressionVirtualColumn(
+ "v1",
+ "string_to_array(\"dim1\",'\\u005C.')",
+ ColumnType.STRING_ARRAY
+ ),
+ expressionVirtualColumn(
+ "v2",
+ "concat(\"_j0.unnest\",'xx')",
+ ColumnType.STRING
+ )
+ )
+ .filters(in("j0.unnest", ImmutableList.of("1", "2"), null))
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .legacy(false)
+ .context(QUERY_CONTEXT_UNNEST)
+ .columns(ImmutableList.of("dim1", "j0.unnest", "v0", "v1", "v2"))
+ .build()
+ ),
+ ImmutableList.of(
+ new Object[]{"10.1", ImmutableList.of("b", "c"), ImmutableList.of("10", "1"), "1", "bxx"},
+ new Object[]{"10.1", ImmutableList.of("b", "c"), ImmutableList.of("10", "1"), "1", "cxx"},
+ new Object[]{"2", ImmutableList.of("d"), ImmutableList.of("2"), "2", "dxx"},
+ new Object[]{"1", null, ImmutableList.of("1"), "1", "xx"}
Review Comment:
Replace with `new Object[]{"1", useDefault ? null: ImmutableList.of(""), ImmutableList.of("1"), "1", "xx"}`
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1131557916
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -226,9 +247,146 @@ public Metadata getMetadata()
return baseAdapter.getMetadata();
}
- public String getDimensionToUnnest()
+ public VirtualColumn getUnnestColumn()
{
- return dimensionToUnnest;
+ return unnestColumn;
}
-}
+ /**
+ * Split queryFilter into pre- and post-correlate filters.
+ *
+ * @param queryFilter query filter passed to makeCursors
+ * @param queryVirtualColumns query virtual columns passed to makeCursors
+ * @param inputColumn input column to unnest if it's a direct access; otherwise null
+ * @param inputColumnCapabilites input column capabilities if known; otherwise null
+ *
+ * @return pair of pre- and post-correlate filters
+ */
+ private Pair<Filter, Filter> computeBaseAndPostCorrelateFilters(
+ @Nullable final Filter queryFilter,
+ final VirtualColumns queryVirtualColumns,
+ @Nullable final String inputColumn,
+ @Nullable final ColumnCapabilities inputColumnCapabilites
+ )
+ {
+ class FilterSplitter
+ {
+ final List<Filter> preFilters = new ArrayList<>();
+ final List<Filter> postFilters = new ArrayList<>();
+
+ void add(@Nullable final Filter filter)
+ {
+ if (filter == null) {
+ return;
+ }
+
+ final Set<String> requiredColumns = filter.getRequiredColumns();
+
+ // Run filter post-correlate if it refers to any virtual columns.
+ if (queryVirtualColumns.getVirtualColumns().length > 0) {
+ for (String column : requiredColumns) {
+ if (queryVirtualColumns.exists(column)) {
+ postFilters.add(filter);
+ return;
+ }
+ }
+ }
+
+ if (requiredColumns.contains(outputColumnName)) {
+ // Try to move filter pre-correlate if possible.
+ final Filter newFilter = rewriteFilterOnUnnestColumnIfPossible(filter, inputColumn, inputColumnCapabilites);
+ if (newFilter != null) {
+ preFilters.add(newFilter);
Review Comment:
actually I think we want to always add the filter as a post filter even when we push down since the pushed down filter returns the whole row if any values match, so we want the value matcher to clean it up to exact matches
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on PR #13892:
URL: https://github.com/apache/druid/pull/13892#issuecomment-1462996702
Additionally there is a CI failure for this test `testGroupByFloatMinExpressionVsVirtualColumnWithExplicitStringVirtualColumnTypedInput`.
The problem seems to trigger from running with `-Ddruid.generic.useDefaultValueForNull=false`. The fix for this is to change the lines
```
"minVc",
NullHandling.replaceWithDefault() ? Float.POSITIVE_INFINITY : null
```
with
```
"minVc",
Float.POSITIVE_INFINITY
```
Although I am unsure what caused this to 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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on PR #13892:
URL: https://github.com/apache/druid/pull/13892#issuecomment-1462640779
Thanks, there is also an unused import in `DruidQueryRel` which would also come in the checkstyle fix.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1132571045
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -226,9 +247,146 @@ public Metadata getMetadata()
return baseAdapter.getMetadata();
}
- public String getDimensionToUnnest()
+ public VirtualColumn getUnnestColumn()
{
- return dimensionToUnnest;
+ return unnestColumn;
}
-}
+ /**
+ * Split queryFilter into pre- and post-correlate filters.
+ *
+ * @param queryFilter query filter passed to makeCursors
+ * @param queryVirtualColumns query virtual columns passed to makeCursors
+ * @param inputColumn input column to unnest if it's a direct access; otherwise null
+ * @param inputColumnCapabilites input column capabilities if known; otherwise null
+ *
+ * @return pair of pre- and post-correlate filters
+ */
+ private Pair<Filter, Filter> computeBaseAndPostCorrelateFilters(
+ @Nullable final Filter queryFilter,
+ final VirtualColumns queryVirtualColumns,
+ @Nullable final String inputColumn,
+ @Nullable final ColumnCapabilities inputColumnCapabilites
+ )
+ {
+ class FilterSplitter
+ {
+ final List<Filter> preFilters = new ArrayList<>();
+ final List<Filter> postFilters = new ArrayList<>();
+
+ void add(@Nullable final Filter filter)
+ {
+ if (filter == null) {
+ return;
+ }
+
+ final Set<String> requiredColumns = filter.getRequiredColumns();
+
+ // Run filter post-correlate if it refers to any virtual columns.
+ if (queryVirtualColumns.getVirtualColumns().length > 0) {
+ for (String column : requiredColumns) {
+ if (queryVirtualColumns.exists(column)) {
+ postFilters.add(filter);
+ return;
+ }
+ }
+ }
+
+ if (requiredColumns.contains(outputColumnName)) {
+ // Try to move filter pre-correlate if possible.
+ final Filter newFilter = rewriteFilterOnUnnestColumnIfPossible(filter, inputColumn, inputColumnCapabilites);
+ if (newFilter != null) {
+ preFilters.add(newFilter);
Review Comment:
PR with the fix is at: https://github.com/apache/druid/pull/13919. If you want to incorporate this into your own work then feel free and I will close my PR.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1127821972
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidCorrelateUnnestRule.java:
##########
@@ -44,134 +51,168 @@
* This class creates the rule to abide by for creating correlations during unnest.
* Typically, Calcite plans the unnest query such as
* SELECT * from numFoo, unnest(dim3) in the following way:
+ *
+ * <pre>
* 80:LogicalCorrelate(correlation=[$cor0], joinType=[inner], requiredColumns=[{3}])
* 6:LogicalTableScan(subset=[rel#74:Subset#0.NONE.[]], table=[[druid, numfoo]])
* 78:Uncollect(subset=[rel#79:Subset#3.NONE.[]])
* 76:LogicalProject(subset=[rel#77:Subset#2.NONE.[]], EXPR$0=[MV_TO_ARRAY($cor0.dim3)])
* 7:LogicalValues(subset=[rel#75:Subset#1.NONE.[0]], tuples=[[{ 0 }]])
+ * </pre>
*
- * {@link DruidUnnestDatasourceRule} takes care of the Uncollect(last 3 lines) to generate a {@link DruidUnnestDatasourceRel}
- * thereby reducing the logical plan to:
+ * {@link DruidUnnestRule} takes care of the Uncollect(last 3 lines) to generate a {@link DruidUnnestRel}
+ * thereby reducing the logical plan to:
+ * <pre>
* LogicalCorrelate
* / \
* DruidRel DruidUnnestDataSourceRel
Review Comment:
nit: this javadoc needs updated i suppose
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java:
##########
@@ -76,6 +76,8 @@ public class CastOperatorConversion implements SqlOperatorConversion
builder.put(type, ExprType.LONG);
}
+ builder.put(SqlTypeName.ARRAY, ExprType.ARRAY);
Review Comment:
this isn't probably that cool because it loses the array element type. I think maybe #13890 has a better solution for this by switching cast to use `Calcites.getColumnTypeForRelDataType`
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1130911456
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidUnnestRel.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.RelWriter;
+import org.apache.calcite.rel.core.Uncollect;
+import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.logical.LogicalValues;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+
+import javax.annotation.Nullable;
+import java.util.Collections;
+import java.util.Set;
+
+/**
+ * Captures an unnest expression for a correlated join. Derived from an {@link Uncollect}.
+ *
+ * This rel cannot be executed directly. It is a holder of information for {@link DruidCorrelateUnnestRel}.
+ *
+ * Unnest on literal values, without correlated join, is handled directly by
+ * {@link org.apache.druid.sql.calcite.rule.DruidUnnestRule}. is applied without a correlated join, This covers the case where an unnest has an
+ * input table, this rel resolves the unnest part and delegates the rel to be consumed by other
+ * rule ({@link org.apache.druid.sql.calcite.rule.DruidCorrelateUnnestRule}
+ */
+public class DruidUnnestRel extends DruidRel<DruidUnnestRel>
Review Comment:
## No clone method
No clone method, yet implements Cloneable.
[Show more details](https://github.com/apache/druid/security/code-scanning/4390)
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #13892: Various changes and fixes to UNNEST.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #13892:
URL: https://github.com/apache/druid/pull/13892#issuecomment-1463091050
> Although I am unsure what caused this to change
Ah. That's because ExpressionVirtualColumn now becomes a pass-through in cases where the expression doesn't do anything. So the behavior of `min` and `minVc` are now the same. I updated the test case to reflect the new expected behavior.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1132105191
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -226,9 +247,146 @@ public Metadata getMetadata()
return baseAdapter.getMetadata();
}
- public String getDimensionToUnnest()
+ public VirtualColumn getUnnestColumn()
{
- return dimensionToUnnest;
+ return unnestColumn;
}
-}
+ /**
+ * Split queryFilter into pre- and post-correlate filters.
+ *
+ * @param queryFilter query filter passed to makeCursors
+ * @param queryVirtualColumns query virtual columns passed to makeCursors
+ * @param inputColumn input column to unnest if it's a direct access; otherwise null
+ * @param inputColumnCapabilites input column capabilities if known; otherwise null
+ *
+ * @return pair of pre- and post-correlate filters
+ */
+ private Pair<Filter, Filter> computeBaseAndPostCorrelateFilters(
+ @Nullable final Filter queryFilter,
+ final VirtualColumns queryVirtualColumns,
+ @Nullable final String inputColumn,
+ @Nullable final ColumnCapabilities inputColumnCapabilites
+ )
+ {
+ class FilterSplitter
+ {
+ final List<Filter> preFilters = new ArrayList<>();
+ final List<Filter> postFilters = new ArrayList<>();
+
+ void add(@Nullable final Filter filter)
+ {
+ if (filter == null) {
+ return;
+ }
+
+ final Set<String> requiredColumns = filter.getRequiredColumns();
+
+ // Run filter post-correlate if it refers to any virtual columns.
+ if (queryVirtualColumns.getVirtualColumns().length > 0) {
+ for (String column : requiredColumns) {
+ if (queryVirtualColumns.exists(column)) {
+ postFilters.add(filter);
+ return;
+ }
+ }
+ }
+
+ if (requiredColumns.contains(outputColumnName)) {
+ // Try to move filter pre-correlate if possible.
+ final Filter newFilter = rewriteFilterOnUnnestColumnIfPossible(filter, inputColumn, inputColumnCapabilites);
+ if (newFilter != null) {
+ preFilters.add(newFilter);
Review Comment:
Oops, thanks for pointing this out. I've written a patch to fix this. In the interests of having this PR pass CI so @somu-imply can keep working on top of it, I'm ok to commit this PR first, and then do another PR fixing 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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] somu-imply commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1131756550
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidRel.java:
##########
@@ -95,6 +96,26 @@ public PlannerContext getPlannerContext()
return plannerContext;
}
+ /**
+ * Overridden to ensure that subclasses provide a proper implementation. The default implementation from
+ * {@link AbstractRelNode} does nothing and is not appropriate.
+ */
+ @Override
+ public RelWriter explainTerms(RelWriter pw)
+ {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ protected Object clone() throws CloneNotSupportedException
+ {
+ // RelNode implements Cloneable, but our class of rels is not cloned, so does not need to implement clone().
+ throw new UnsupportedOperationException();
Review Comment:
IntelliJ inspections is flagging it as it does not throw a CloneNotSupportedException
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1130948373
##########
processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java:
##########
@@ -226,9 +247,146 @@ public Metadata getMetadata()
return baseAdapter.getMetadata();
}
- public String getDimensionToUnnest()
+ public VirtualColumn getUnnestColumn()
{
- return dimensionToUnnest;
+ return unnestColumn;
}
-}
+ /**
+ * Split queryFilter into pre- and post-correlate filters.
+ *
+ * @param queryFilter query filter passed to makeCursors
+ * @param queryVirtualColumns query virtual columns passed to makeCursors
+ * @param inputColumn input column to unnest if it's a direct access; otherwise null
+ * @param inputColumnCapabilites input column capabilities if known; otherwise null
+ *
+ * @return pair of pre- and post-correlate filters
+ */
+ private Pair<Filter, Filter> computeBaseAndPostCorrelateFilters(
+ @Nullable final Filter queryFilter,
+ final VirtualColumns queryVirtualColumns,
+ @Nullable final String inputColumn,
+ @Nullable final ColumnCapabilities inputColumnCapabilites
+ )
+ {
+ class FilterSplitter
+ {
+ final List<Filter> preFilters = new ArrayList<>();
+ final List<Filter> postFilters = new ArrayList<>();
+
+ void add(@Nullable final Filter filter)
+ {
+ if (filter == null) {
+ return;
+ }
+
+ final Set<String> requiredColumns = filter.getRequiredColumns();
+
+ // Run filter post-correlate if it refers to any virtual columns.
+ if (queryVirtualColumns.getVirtualColumns().length > 0) {
+ for (String column : requiredColumns) {
+ if (queryVirtualColumns.exists(column)) {
+ postFilters.add(filter);
+ return;
+ }
+ }
+ }
+
+ if (requiredColumns.contains(outputColumnName)) {
+ // Try to move filter pre-correlate if possible.
+ final Filter newFilter = rewriteFilterOnUnnestColumnIfPossible(filter, inputColumn, inputColumnCapabilites);
+ if (newFilter != null) {
+ preFilters.add(newFilter);
+ } else {
+ postFilters.add(filter);
+ }
Review Comment:
nice :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1131378470
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidUnnestRel.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.RelWriter;
+import org.apache.calcite.rel.core.Uncollect;
+import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.logical.LogicalValues;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+
+import javax.annotation.Nullable;
+import java.util.Collections;
+import java.util.Set;
+
+/**
+ * Captures an unnest expression for a correlated join. Derived from an {@link Uncollect}.
+ *
+ * This rel cannot be executed directly. It is a holder of information for {@link DruidCorrelateUnnestRel}.
+ *
+ * Unnest on literal values, without correlated join, is handled directly by
+ * {@link org.apache.druid.sql.calcite.rule.DruidUnnestRule}. is applied without a correlated join, This covers the case where an unnest has an
+ * input table, this rel resolves the unnest part and delegates the rel to be consumed by other
+ * rule ({@link org.apache.druid.sql.calcite.rule.DruidCorrelateUnnestRule}
+ */
+public class DruidUnnestRel extends DruidRel<DruidUnnestRel>
Review Comment:
Oh, I didn't realize RelNode implements Cloneable. This hasn't been an issue for any of the other DruidRels; none of them implement `clone`. Calcite's builtin rels don't either, except for MutableRel, which isn't involved here. I think it's fine to not implement this. I'll add an impl to `DruidRel` that throws an exception and write a comment remarking that it isn't needed.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13892: Various changes and fixes to UNNEST.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13892:
URL: https://github.com/apache/druid/pull/13892#discussion_r1129884367
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidCorrelateUnnestRule.java:
##########
@@ -44,134 +51,168 @@
* This class creates the rule to abide by for creating correlations during unnest.
* Typically, Calcite plans the unnest query such as
* SELECT * from numFoo, unnest(dim3) in the following way:
+ *
+ * <pre>
* 80:LogicalCorrelate(correlation=[$cor0], joinType=[inner], requiredColumns=[{3}])
* 6:LogicalTableScan(subset=[rel#74:Subset#0.NONE.[]], table=[[druid, numfoo]])
* 78:Uncollect(subset=[rel#79:Subset#3.NONE.[]])
* 76:LogicalProject(subset=[rel#77:Subset#2.NONE.[]], EXPR$0=[MV_TO_ARRAY($cor0.dim3)])
* 7:LogicalValues(subset=[rel#75:Subset#1.NONE.[0]], tuples=[[{ 0 }]])
+ * </pre>
*
- * {@link DruidUnnestDatasourceRule} takes care of the Uncollect(last 3 lines) to generate a {@link DruidUnnestDatasourceRel}
- * thereby reducing the logical plan to:
+ * {@link DruidUnnestRule} takes care of the Uncollect(last 3 lines) to generate a {@link DruidUnnestRel}
+ * thereby reducing the logical plan to:
+ * <pre>
* LogicalCorrelate
* / \
* DruidRel DruidUnnestDataSourceRel
Review Comment:
Yep. Updated it.
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java:
##########
@@ -76,6 +76,8 @@ public class CastOperatorConversion implements SqlOperatorConversion
builder.put(type, ExprType.LONG);
}
+ builder.put(SqlTypeName.ARRAY, ExprType.ARRAY);
Review Comment:
Yeah, yours is better. Merged it in. 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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org