abhishekrb19 commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3806797518
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/simulate/EmbeddedDimensionValueSetShardSpecTest.java:
##########
@@ -277,26 +277,24 @@ public void
test_multiDimensionAndMultiValuePartitionDimensionValues()
}
/**
- * Numeric (Long) tracked dimensions are recorded in the shard spec but are
NOT used for pruning: numeric query
- * filters opt out of segment pruning (their getDimensionRangeSet returns
null, since pruning compares values
- * lexicographically), so a numeric equality filter scans every segment even
though each segment declares exactly
- * one numeric value. Queries stay correct; there is simply no pruning
benefit.
- *
- * <p>This is intentional for now. We could either consider extending
pruning to numeric types with type-aware
- * (non-lexicographic) comparison, or (b) reject numeric dimensions outright
when they're declared.
+ * A numeric (LONG) tracked dimension with an explicit {@link
LongDimensionSchema} IS pruned: the broker's
+ * equality/IN/IS NULL channel is type-gated on {@code
dimensionColumnTypes}, which the task stamps only for such a
+ * schema (a LONG stringifies identically on ingest and query, so set
membership is exact). This is exact
+ * set-membership, not a range comparison, so it works even though the
dimension is numeric.
*/
@Test
- public void test_numericDimension_isNotPruned()
+ public void test_numericLongDimension_isPruned()
{
Review Comment:
Could you update the docs to also note long types are supported as well:
https://github.com/apache/druid/blob/master/docs/ingestion/kafka-ingestion.md#streaming-partitions-spec:
>Only string-typed dimensions are currently supported.
##########
processing/src/test/java/org/apache/druid/timeline/partition/DimensionValueSetShardSpecTest.java:
##########
@@ -310,4 +333,245 @@ public void testEmptyAllowedList_prunesEverything()
final DimensionValueSetShardSpec s = spec(ImmutableMap.of(TENANT,
List.of()));
Assertions.assertFalse(s.possibleInDomain(domain(TENANT, "tenant_a")));
}
+
+ @Test
+ public void testStringDomain_skipsTypeStampedDimension()
+ {
+ // A LONG-stamped dim holds canonicalized values ("1"); a non-canonical
selector like code = "00001" matches
+ // the indexed LONG via coercion, so the literal string range must not
prune this segment.
+ final DimensionValueSetShardSpec s = spec(
+ ImmutableMap.of(CODE, List.of("1")),
+ ImmutableMap.of(CODE, ColumnType.LONG)
+ );
+ Assertions.assertTrue(s.possibleInDomain(domain(CODE, "00001")));
+ }
+
+ @Test
+ public void testNullDomain_prunesTypeStampedDimensionWithoutNull()
+ {
+ // IS NULL yields no typed value set, so it is pruned here (not by
possibleInValueDomain): a segment with no null
+ // observed cannot match.
+ final DimensionValueSetShardSpec s = spec(
+ ImmutableMap.of(CODE, List.of("1", "2")),
+ ImmutableMap.of(CODE, ColumnType.LONG)
+ );
+ Assertions.assertFalse(s.possibleInDomain(nullDomain(CODE)));
+ }
+
+ @Test
+ public void testNullDomain_keepsTypeStampedDimensionThatObservedNull()
+ {
+ // A LONG-stamped dim that observed a null value must be kept for an IS
NULL query.
+ final DimensionValueSetShardSpec s = spec(
+ ImmutableMap.of(CODE, Arrays.asList("1", null)),
+ ImmutableMap.of(CODE, ColumnType.LONG)
+ );
+ Assertions.assertTrue(s.possibleInDomain(nullDomain(CODE)));
+ }
+
+ @Test
+ public void
testMixedNullAndValueDomain_defersTypeStampedDimensionToValueChannel()
Review Comment:
Claude notes:
`testMixedNullAndValueDomain_defersTypeStampedDimensionToValueChannel` reads
like it proves the OR+NULL case is handled by the value channel, but it only
checks possibleInDomain in
isolation returning true ("not pruned here") — it never wires that into
`possibleInValueDomain` with a real filter to confirm the value channel
actually picks up the slack. Since I traced that it currently doesn't (because
`OrDimFilter.getDimensionValueSet` bails to null when the IS NULL branch
can't produce a value set), there's no test actually asserting end-to-end
behavior for this compound shape.
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/simulate/EmbeddedDimensionValueSetShardSpecTest.java:
##########
@@ -341,20 +339,49 @@ public void test_numericDimension_isNotPruned()
awaitRowsProcessed(6);
suspendAndAwaitHandoff(spec, 1);
- // The numeric value IS recorded in the shard spec (stringified), even
though it is never used for pruning.
verifyAllSegmentsHaveDimensionValueSetShardSpec(dataSource);
+ // The published shard spec stamps colCode's type as LONG, gating the
broker's numeric pruning channel.
+ final List<Map<String, Object>> shardSpecs = getShardSpecs(dataSource);
+ for (Map<String, Object> shardSpec : shardSpecs) {
+ @SuppressWarnings("unchecked")
+ final Map<String, Object> columnTypes = (Map<String, Object>)
shardSpec.get("dimensionColumnTypes");
+ Assertions.assertNotNull(columnTypes, "Expected dimensionColumnTypes on
shard spec: " + shardSpec);
+ Assertions.assertEquals(
+ "LONG",
+ columnTypes.get(colCode),
+ "Expected " + colCode + " stamped as LONG in dimensionColumnTypes: "
+ shardSpec
+ );
+ }
+
final Map<String, String> startToSegmentId =
getStartToSegmentId(dataSource);
final String day1 = startToSegmentId.get("2025-01-01T00:00:00.000Z");
final String day2 = startToSegmentId.get("2025-01-02T00:00:00.000Z");
final String day3 = startToSegmentId.get("2025-01-03T00:00:00.000Z");
- final Set<String> allDays = Set.of(day1, day2, day3);
+ Assertions.assertNotNull(day1, "Missing Day1 segment id in: " +
startToSegmentId);
+ Assertions.assertNotNull(day2, "Missing Day2 segment id in: " +
startToSegmentId);
+ Assertions.assertNotNull(day3, "Missing Day3 segment id in: " +
startToSegmentId);
- // Correct count, but ALL segments scanned: numeric equality does not
prune even though each segment holds one code.
- assertScan("2", allDays, "SELECT COUNT(*) FROM %s WHERE %s = 1",
dataSource, colCode);
- assertScan("2", allDays, "SELECT COUNT(*) FROM %s WHERE %s = 3",
dataSource, colCode);
- // A non-existent code returns 0 rows but is still NOT pruned (would be a
full prune if numeric pruning worked).
- assertScan("0", allDays, "SELECT COUNT(*) FROM %s WHERE %s = 999",
dataSource, colCode);
+ // Day1's non-canonical tokens ("00001", "+1") are stamped as the
canonical Long string "1", not verbatim.
+ @SuppressWarnings("unchecked")
+ final Set<String> allCodes = shardSpecs.stream()
+ .map(shardSpec -> ((Map<String, List<String>>)
shardSpec.get("partitionDimensionValues")).get(colCode))
+ .flatMap(List::stream)
+ .collect(Collectors.toSet());
+ Assertions.assertEquals(Set.of("1", "2", "3"), allCodes, "Expected
canonical LONG codes in partitionDimensionValues");
+
+ // Numeric equality now prunes exactly like the string case: each query
scans only the one segment whose
+ // stamped value set contains the queried code.
+ assertScan("2", Set.of(day1), "SELECT COUNT(*) FROM %s WHERE %s = 1",
dataSource, colCode);
+ assertScan("2", Set.of(day3), "SELECT COUNT(*) FROM %s WHERE %s = 3",
dataSource, colCode);
Review Comment:
Nice 👍
##########
processing/src/main/java/org/apache/druid/timeline/partition/DimensionValueSetShardSpec.java:
##########
@@ -44,15 +45,44 @@ public class DimensionValueSetShardSpec extends
NumberedShardSpec
*/
private final Map<String, List<String>> partitionDimensionValues;
+ /**
+ * Maps dimension name → the {@link ColumnType} of the values stamped in
{@link #partitionDimensionValues} for that
+ * dimension. Only populated for types safe for typed value-set pruning
(currently {@link ColumnType#LONG}); a
+ * dimension absent from this map is pruned only via the string/range
channel ({@link #possibleInDomain(Map)}),
+ * never via {@link #possibleInValueDomain(Map)}.
+ *
+ * <p>This is the safety gate for numeric pruning: {@link
#possibleInValueDomain(Map)} may only prune a dimension
+ * when the query filter's match type equals the type recorded here, because
value stringification only agrees
+ * across ingest and query for identical types.
+ */
+ private final Map<String, ColumnType> dimensionColumnTypes;
+
@JsonCreator
public DimensionValueSetShardSpec(
@JsonProperty("partitionNum") int partitionNum,
@JsonProperty("partitions") int partitions,
- @JsonProperty("partitionDimensionValues") @Nullable Map<String,
List<String>> partitionDimensionValues
+ @JsonProperty("partitionDimensionValues") @Nullable Map<String,
List<String>> partitionDimensionValues,
Review Comment:
Would it be feasible to actually store the raw types as-is by leveraging
the column types captured below in the filters? Currently we seem to stringify
the values and coerce them again when its filtered.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/DimensionValueSetCollector.java:
##########
Review Comment:
Perhaps can we document the types applicable for pruning somewhere here?
(only string and long)
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/simulate/EmbeddedDimensionValueSetShardSpecTest.java:
##########
@@ -277,26 +277,24 @@ public void
test_multiDimensionAndMultiValuePartitionDimensionValues()
}
/**
- * Numeric (Long) tracked dimensions are recorded in the shard spec but are
NOT used for pruning: numeric query
- * filters opt out of segment pruning (their getDimensionRangeSet returns
null, since pruning compares values
- * lexicographically), so a numeric equality filter scans every segment even
though each segment declares exactly
- * one numeric value. Queries stay correct; there is simply no pruning
benefit.
- *
- * <p>This is intentional for now. We could either consider extending
pruning to numeric types with type-aware
- * (non-lexicographic) comparison, or (b) reject numeric dimensions outright
when they're declared.
+ * A numeric (LONG) tracked dimension with an explicit {@link
LongDimensionSchema} IS pruned: the broker's
+ * equality/IN/IS NULL channel is type-gated on {@code
dimensionColumnTypes}, which the task stamps only for such a
+ * schema (a LONG stringifies identically on ingest and query, so set
membership is exact). This is exact
+ * set-membership, not a range comparison, so it works even though the
dimension is numeric.
*/
@Test
- public void test_numericDimension_isNotPruned()
Review Comment:
Could we add a test each for float and double here? So when those types are
supported the behavior would change 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: [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]
