clintropolis commented on code in PR #14587:
URL: https://github.com/apache/druid/pull/14587#discussion_r1284739869
##########
processing/src/test/java/org/apache/druid/segment/UnnestStorageAdapterTest.java:
##########
@@ -392,6 +564,35 @@ public void
test_pushdown_filters_unnested_dimension_outside()
return null;
});
}
+
+ public void testComputeBaseAndPostUnnestFilters(
+ Filter testQueryFilter,
+ String expectedBasePushDown,
+ String expectedPostUnnest
+ )
+ {
+ final String inputColumn =
UNNEST_STORAGE_ADAPTER.getUnnestInputIfDirectAccess(UNNEST_STORAGE_ADAPTER.getUnnestColumn());
+ final VirtualColumn vc = UNNEST_STORAGE_ADAPTER.getUnnestColumn();
+ Pair<Filter, Filter> filterPair =
UNNEST_STORAGE_ADAPTER.computeBaseAndPostUnnestFilters(
+ testQueryFilter,
+ null,
+ VirtualColumns.EMPTY,
+ inputColumn,
+ vc.capabilities(inputColumn)
Review Comment:
this should be
```suggestion
vc.capabilities(UNNEST_STORAGE_ADAPTER, inputColumn)
```
##########
processing/src/test/java/org/apache/druid/segment/UnnestStorageAdapterTest.java:
##########
@@ -300,7 +306,175 @@ public void
test_pushdown_or_filters_unnested_and_original_dimension_with_unnest
return null;
});
}
+ @Test
+ public void
test_nested_filters_unnested_and_original_dimension_with_unnest_adapters()
+ {
+ final UnnestStorageAdapter unnestStorageAdapter = new UnnestStorageAdapter(
+ new TestStorageAdapter(INCREMENTAL_INDEX),
+ new ExpressionVirtualColumn(OUTPUT_COLUMN_NAME, "\"" + COLUMNNAME +
"\"", null, ExprMacroTable.nil()),
+ null
+ );
+
+ final VirtualColumn vc = unnestStorageAdapter.getUnnestColumn();
+
+ final String inputColumn =
unnestStorageAdapter.getUnnestInputIfDirectAccess(vc);
+
+ final OrFilter baseFilter = new OrFilter(ImmutableList.of(
+ selector(OUTPUT_COLUMN_NAME, "1"),
+ new AndFilter(ImmutableList.of(
+ selector(inputColumn, "2"),
+ selector(OUTPUT_COLUMN_NAME, "10")
+ ))
+ ));
+
+ final OrFilter expectedPushDownFilter = new OrFilter(ImmutableList.of(
+ selector(inputColumn, "1"),
+ new AndFilter(ImmutableList.of(
+ selector(inputColumn, "2"),
+ selector(inputColumn, "10")
+ ))
+ ));
+
+ final Sequence<Cursor> cursorSequence = unnestStorageAdapter.makeCursors(
+ baseFilter,
+ unnestStorageAdapter.getInterval(),
+ VirtualColumns.EMPTY,
+ Granularities.ALL,
+ false,
+ null
+ );
+
+ final TestStorageAdapter base = (TestStorageAdapter)
unnestStorageAdapter.getBaseAdapter();
+ final Filter pushDownFilter = base.getPushDownFilter();
+
+ Assert.assertEquals(expectedPushDownFilter, pushDownFilter);
+ cursorSequence.accumulate(null, (accumulated, cursor) -> {
+ Assert.assertEquals(cursor.getClass(), PostJoinCursor.class);
+ final Filter postFilter = ((PostJoinCursor) cursor).getPostJoinFilter();
+ // OR-case so base filter should match the postJoinFilter
+ Assert.assertEquals(baseFilter, postFilter);
+ return null;
+ });
+ }
+ @Test
+ public void test_nested_filters_unnested_and_topLevel1And3filtersInOR()
+ {
+ final Filter testQueryFilter = and(ImmutableList.of(
+ selector(OUTPUT_COLUMN_NAME, "3"),
+ or(ImmutableList.of(
+ selector("newcol", "2"),
+ selector(COLUMNNAME, "2"),
+ selector(OUTPUT_COLUMN_NAME, "1")
+ ))
+ ));
+ testComputeBaseAndPostUnnestFilters(
+ testQueryFilter,
+ "unnested-multi-string1 = 3",
+ "(unnested-multi-string1 = 3 && (newcol = 2 || multi-string1 = 2 ||
unnested-multi-string1 = 1))"
+ );
+ }
+ @Test
+ public void test_nested_multiLevel_filters_unnested()
+ {
+ final Filter testQueryFilter = and(ImmutableList.of(
+ selector(OUTPUT_COLUMN_NAME, "3"),
+ or(ImmutableList.of(
+ or(ImmutableList.of(
+ selector("newcol", "2"),
+ selector(COLUMNNAME, "2"),
+ and(ImmutableList.of(
+ selector("newcol", "3"),
+ selector(COLUMNNAME, "7")
+ ))
+ )),
+ selector(OUTPUT_COLUMN_NAME, "1")
+ ))
+ ));
+ testComputeBaseAndPostUnnestFilters(
+ testQueryFilter,
+ "unnested-multi-string1 = 3",
+ "(unnested-multi-string1 = 3 && (newcol = 2 || multi-string1 = 2 ||
(newcol = 3 && multi-string1 = 7) || unnested-multi-string1 = 1))"
+ );
+ }
+ @Test
+ public void test_nested_multiLevel_filters_unnested5Level()
+ {
+ final Filter testQueryFilter = or(ImmutableList.of(
+ selector(OUTPUT_COLUMN_NAME, "3"),
+ or(ImmutableList.of(
+ or(ImmutableList.of(
+ selector("newcol", "2"),
+ selector(COLUMNNAME, "2"),
+ and(ImmutableList.of(
+ selector("newcol", "3"),
+ and(ImmutableList.of(
+ selector(COLUMNNAME, "7"),
+ selector("newcol_1", "10")
+ ))
+ ))
+ )),
+ selector(OUTPUT_COLUMN_NAME, "1")
+ ))
+ ));
+ testComputeBaseAndPostUnnestFilters(
+ testQueryFilter,
+ "",
+ "(unnested-multi-string1 = 3 || newcol = 2 || multi-string1 = 2 ||
(newcol = 3 && multi-string1 = 7 && newcol_1 = 10) || unnested-multi-string1 =
1)"
+ );
+ }
+ @Test
+ public void test_nested_filters_unnested_and_topLevelORAnd3filtersInOR()
+ {
+ final Filter testQueryFilter = or(ImmutableList.of(
+ selector(OUTPUT_COLUMN_NAME, "3"),
+ and(ImmutableList.of(
+ selector("newcol", "2"),
+ selector(COLUMNNAME, "2"),
+ selector(OUTPUT_COLUMN_NAME, "1")
+ ))
+ ));
+ testComputeBaseAndPostUnnestFilters(
+ testQueryFilter,
+ "",
+ "(unnested-multi-string1 = 3 || (newcol = 2 && multi-string1 = 2 &&
unnested-multi-string1 = 1))"
+ );
+ }
+ @Test
+ public void
test_nested_filters_unnested_and_topLevelAND3filtersInORWithNestedOrs()
+ {
+ final Filter testQueryFilter = and(ImmutableList.of(
+ selector(OUTPUT_COLUMN_NAME, "3"),
+ or(ImmutableList.of(
+ selector("newcol", "2"),
+ selector(COLUMNNAME, "2")
+ )),
+ or(ImmutableList.of(
+ selector("newcol", "4"),
+ selector(COLUMNNAME, "8"),
+ selector(OUTPUT_COLUMN_NAME, "6")
+ ))
+ ));
+ testComputeBaseAndPostUnnestFilters(
+ testQueryFilter,
+ "unnested-multi-string1 = 3",
+ "(unnested-multi-string1 = 3 && (newcol = 2 || multi-string1 = 2) &&
(newcol = 4 || multi-string1 = 8 || unnested-multi-string1 = 6))"
+ );
+ }
+
+ @Test
+ public void test_nested_filters_unnested_and_topLevelAND2sdf()
+ {
+ final Filter testQueryFilter = and(ImmutableList.of(
+ selector(OUTPUT_COLUMN_NAME, "3"),
+ selector(COLUMNNAME, "2")
+ ));
+ testComputeBaseAndPostUnnestFilters(
+ testQueryFilter,
+ "unnested-multi-string1 = 3",
Review Comment:
this doesn't seem right, shouldn't it be pushing down `multi-string1 = 3`
since `unnested-multi-string1` doesn't exist on the base adapter? I think the
problem is that `testComputeBaseAndPostUnnestFilters` is not getting the
virtual column capabilities correctly, which makes it use the default
capabilities to define it as float typed rather than string typed, so it isn't
rewritten correctly in `rewriteFilterOnUnnestColumnIfPossible` because it is
not string typed.
If i change the line to get the capabilities from the unnest storage adapter
```
vc.capabilities(UNNEST_STORAGE_ADAPTER, inputColumn)
```
, the test fails because the pushed down filter is ```(multi-string1 = 3 &&
multi-string1 = 2)``` which seems like the correct answer? Also makes most of
the other tests fail expectations. I think we should probably add a check to
ensure that filters on the unnest column never get pushed down, since I think
that would effectively make the results null since the filter would match
nothing since the column doesn't exist (unless it was filtering for nulls i
guess)
--
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]
