gianm commented on code in PR #20149:
URL: https://github.com/apache/druid/pull/20149#discussion_r3859405052
##########
processing/src/main/java/org/apache/druid/query/filter/FilterSegmentPruner.java:
##########
@@ -164,10 +201,37 @@ public String toString()
'}';
}
+ /**
+ * Adds the filter's {@link RangeSet} for {@code column} to {@code
filterDomain}, resolving through
+ * {@code domainVirtualColumns} to the query's equivalent virtual column if
{@code column} is virtual there.
+ */
+ private void addToFilterDomain(
+ String column,
+ VirtualColumns domainVirtualColumns,
+ Map<String, RangeSet<String>> filterDomain
+ )
+ {
+ final VirtualColumns.Node domainNode =
domainVirtualColumns.getNode(column);
+ if (domainNode != null) {
+ final VirtualColumn queryEquivalent = getQueryEquivalent(domainNode);
+ if (queryEquivalent != null &&
filterFields.contains(queryEquivalent.getOutputName())) {
Review Comment:
I think that cluster group virtual columns end up being directly queryable.
If this is right then the matching should apply to both the virtual column and
to the materialized column (i.e. `WHERE group_key = 'something'`, where
`group_key` is the name of the materialized cluster column.)
##########
processing/src/test/java/org/apache/druid/query/filter/FilterSegmentPrunerTest.java:
##########
@@ -329,6 +331,74 @@ void testPruneNumericIn()
Assertions.assertTrue(pruner.include(seg));
}
+ @Test
+ void testPruneClusterGroupTuples()
+ {
+ final String interval = "2026-01-01T00:00:00Z/2026-01-02T00:00:00Z";
+ final RowSignature clusteringColumns = RowSignature.builder().add("dim1",
ColumnType.STRING).build();
+ final ClusterGroupTuples tuples = new ClusterGroupTuples(
+ clusteringColumns,
+ List.of(List.of("abc"), List.of("xyz"))
+ );
+
+ final DataSegment seg = makeDataSegment(interval, makeRange("dim1", 0,
null, null), tuples);
+
+ final DimFilter matchingFilter = new EqualityFilter("dim1",
ColumnType.STRING, "abc", null);
+ final DimFilter nonMatchingFilter = new EqualityFilter("dim1",
ColumnType.STRING, "foo", null);
+
+ Assertions.assertTrue(new FilterSegmentPruner(matchingFilter, null,
null).include(seg));
+ Assertions.assertFalse(new FilterSegmentPruner(nonMatchingFilter, null,
null).include(seg));
+ }
+
+ @Test
+ void testClusterGroupTuplesSkipsNonStringColumns()
+ {
+ // Numeric columns are skipped for pruning (see druid issue #19408), so
this must not prune.
+ final String interval = "2026-01-01T00:00:00Z/2026-01-02T00:00:00Z";
+ final RowSignature clusteringColumns = RowSignature.builder().add("id",
ColumnType.LONG).build();
+ final ClusterGroupTuples tuples = new
ClusterGroupTuples(clusteringColumns, List.of(List.of(100L), List.of(200L)));
+
+ final DataSegment seg = makeDataSegment(interval, makeRange("dim1", 0,
null, null), tuples);
+ final DimFilter filter = new EqualityFilter("id", ColumnType.LONG, 999L,
null);
+
+ Assertions.assertTrue(new FilterSegmentPruner(filter, null,
null).include(seg));
+ }
+
+ @Test
+ void testPruneClusterGroupTuplesVirtualColumn()
+ {
+ final VirtualColumns clusterVirtualColumns = VirtualColumns.create(
+ new ExpressionVirtualColumn("vdim1", "concat(dim1, 'foo')",
ColumnType.STRING, TestExprMacroTable.INSTANCE)
+ );
+ final RowSignature clusteringColumns = RowSignature.builder().add("vdim1",
ColumnType.STRING).build();
+ final ClusterGroupTuples tuples = new ClusterGroupTuples(
+ clusteringColumns,
+ clusterVirtualColumns,
+ List.of(List.of("abcfoo"), List.of("xyzfoo"))
+ );
+
+ final String interval = "2026-01-01T00:00:00Z/2026-01-02T00:00:00Z";
+ final DataSegment seg = makeDataSegment(interval, makeRange("dim1", 0,
null, null), tuples);
+
+ // same expression, same name
+ VirtualColumns queryVirtualColumns = VirtualColumns.create(
+ new ExpressionVirtualColumn("vdim1", "concat(dim1, 'foo')",
ColumnType.STRING, TestExprMacroTable.INSTANCE)
Review Comment:
Please add a test that confirms `vdim1` set to a different expression never
prunes.
Please also add a test for querying `vdim1` directly when there is no
virtual column with that name. (It should prune when cluster groups are present
on the segment, because in that case `vdim1` is materialized.)
##########
processing/src/test/java/org/apache/druid/query/filter/FilterSegmentPrunerTest.java:
##########
@@ -329,6 +331,74 @@ void testPruneNumericIn()
Assertions.assertTrue(pruner.include(seg));
}
+ @Test
+ void testPruneClusterGroupTuples()
Review Comment:
Please add some tests for a cluster key with multiple columns.
--
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]
