From 19e4a96e6acd61efd851d5f5482192d584850317 Mon Sep 17 00:00:00 2001 From: Soumyava Das Date: Sun, 14 May 2023 22:16:15 -0700 Subject: [PATCH 1/3] Fixing an issue with filtering on a single dimension by converting In filter to a selector filter as needed with Filters.toFilter --- .../apache/druid/segment/join/JoinableFactoryWrapper.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java index 94c98f8ae2b8..df4d14cf621c 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java +++ b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java @@ -30,6 +30,7 @@ import org.apache.druid.query.filter.Filter; import org.apache.druid.query.filter.InDimFilter; import org.apache.druid.segment.filter.FalseFilter; +import org.apache.druid.segment.filter.Filters; import org.apache.druid.utils.CollectionUtils; import javax.annotation.Nullable; @@ -177,7 +178,10 @@ static JoinClauseToFilterConversion convertJoinToFilter( } return new JoinClauseToFilterConversion(null, false); } - final Filter onlyFilter = new InDimFilter(leftColumn, columnValuesWithUniqueFlag.getColumnValues()); + final Filter onlyFilter = Filters.toFilter(new InDimFilter( + leftColumn, + columnValuesWithUniqueFlag.getColumnValues() + )); if (!columnValuesWithUniqueFlag.isAllUnique()) { joinClauseFullyConverted = false; } From 2dd7128f3498c97f66ded12e91ce444c9284efe1 Mon Sep 17 00:00:00 2001 From: Soumyava Das Date: Mon, 15 May 2023 11:42:13 -0700 Subject: [PATCH 2/3] Adding a test so that any future refactoring does not break this behavior --- .../join/JoinableFactoryWrapperTest.java | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java b/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java index 8ff6d8461f8f..845f9fb56cb1 100644 --- a/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java +++ b/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java @@ -36,6 +36,7 @@ import org.apache.druid.segment.column.ColumnType; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.segment.filter.FalseFilter; +import org.apache.druid.segment.filter.SelectorFilter; import org.apache.druid.segment.join.lookup.LookupJoinable; import org.apache.druid.segment.join.table.IndexedTable; import org.apache.druid.segment.join.table.IndexedTableJoinable; @@ -100,6 +101,13 @@ public class JoinableFactoryWrapperTest extends NullHandlingTest .build() ); + private static final InlineDataSource INDEXED_TABLE_DS_ONE_ROW = InlineDataSource.fromIterable( + ImmutableList.of( + new Object[]{"Mexico"} + ), + RowSignature.builder().add("country", ColumnType.STRING).build() + ); + private static final InlineDataSource NULL_INDEXED_TABLE_DS = InlineDataSource.fromIterable( ImmutableList.of( new Object[]{null} @@ -123,6 +131,14 @@ public class JoinableFactoryWrapperTest extends NullHandlingTest DateTimes.nowUtc().toString() ); + private static final IndexedTable TEST_INDEXED_TABLE_ONE_ROW = new RowBasedIndexedTable<>( + INDEXED_TABLE_DS_ONE_ROW.getRowsAsList(), + INDEXED_TABLE_DS_ONE_ROW.rowAdapter(), + INDEXED_TABLE_DS_ONE_ROW.getRowSignature(), + ImmutableSet.of("country"), + DateTimes.nowUtc().toString() + ); + private static final IndexedTable TEST_NULL_INDEXED_TABLE = new RowBasedIndexedTable<>( NULL_INDEXED_TABLE_DS.getRowsAsList(), NULL_INDEXED_TABLE_DS.rowAdapter(), @@ -240,6 +256,33 @@ public void test_convertJoinsToPartialFilters_convertInnerJoin() ); } + @Test + public void test_convertJoinsToPartialFiltersWithSelectorFiltersInsteadOfInsForSingleValue() + { + JoinableClause joinableClause = new JoinableClause( + "j.", + new IndexedTableJoinable(TEST_INDEXED_TABLE_ONE_ROW), + JoinType.INNER, + JoinConditionAnalysis.forExpression("x == \"j.country\"", "j.", ExprMacroTable.nil()) + ); + final Pair, List> conversion = JoinableFactoryWrapper.convertJoinsToFilters( + ImmutableList.of(joinableClause), + ImmutableSet.of("x"), + Integer.MAX_VALUE + ); + + // Although the filter created was an In Filter in equijoin + // We should receive a SelectorFilter for Filters.toFilter + // as there is a single value + Assert.assertEquals( + Pair.of( + ImmutableList.of(new SelectorFilter("x", "Mexico", null)), + ImmutableList.of() + ), + conversion + ); + } + @Test public void test_convertJoinsToFilters_convertTwoInnerJoins() { From b3b9b4491b021d4433cb7ffb38637cdaa36caccf Mon Sep 17 00:00:00 2001 From: Soumyava Das Date: Mon, 15 May 2023 11:48:55 -0700 Subject: [PATCH 3/3] Made comment a bit more meaningful --- .../druid/segment/join/JoinableFactoryWrapperTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java b/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java index 845f9fb56cb1..f4ee1676b599 100644 --- a/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java +++ b/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java @@ -271,9 +271,9 @@ public void test_convertJoinsToPartialFiltersWithSelectorFiltersInsteadOfInsForS Integer.MAX_VALUE ); - // Although the filter created was an In Filter in equijoin - // We should receive a SelectorFilter for Filters.toFilter - // as there is a single value + // Although the filter created was an In Filter in equijoin (here inFilter = IN (Mexico)) + // We should receive a SelectorFilter for Filters.toFilter(inFilter) call + // and should receive a SelectorFilter with x = Mexico Assert.assertEquals( Pair.of( ImmutableList.of(new SelectorFilter("x", "Mexico", null)),