You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "jaykanakiya (via GitHub)" <gi...@apache.org> on 2026/07/20 11:09:33 UTC

[PR] feat: numeric pruning for dim_value_set segments (druid)

jaykanakiya opened a new pull request, #19704:
URL: https://github.com/apache/druid/pull/19704

   <!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
   
   <!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
   
   <!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
   
   <!-- If you are a committer, follow the PR action item checklist for committers:
   https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
   
   ### Description
   
   <!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
   
   <!-- Describe your patch: what did you change in code? How did you fix the problem? -->
   
   <!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
   
   Follow up to [#19571](https://github.com/apache/druid/pull/19571)
   
    Adds  **LONG** equality / `IN` / `IS NULL` segment pruning for `dim_value_set` segments.
   
   <!--
   In each section, please describe design decisions made, including:
    - Choice of algorithms
    - Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
    - Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
    - Method organization and design (how the logic is split between methods, parameters and return types)
    - Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
   -->
   
   
   <!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
   
   <!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
   
   <!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
   
   #### Release note
   <!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users. 
   
   If your change doesn't have end user impact, you can skip this section.
   
   For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
   
   -->
   
   
   <hr>
   
   ##### Key changed/added classes in this PR
    * `DimensionValueSetShardSpec`
    * `SeekableStreamIndexTaskRunner`
    * `TypedValueSet`
   
   <hr>
   
   <!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
   
   This PR has:
   
   - [x] been self-reviewed.
   - [] added documentation for new or modified features or behaviors.
   - [ ] a release note entry in the PR description.
   - [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
   - [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
   - [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   - [ ] added integration tests.
   - [x] been tested in a test Druid cluster.


-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "jaykanakiya (via GitHub)" <gi...@apache.org>.
jaykanakiya commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3619593291


##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java:
##########
@@ -747,13 +755,17 @@ public void run()
                           dim,
                           k -> Collections.synchronizedSet(new HashSet<>())
                       );
-                      // Empty getDimension result means a null/missing value; record null so IS NULL is not pruned
-                      // (distinct from "", which getDimension returns as ["" ]).
-                      final List<String> dimValues = row.getDimension(dim);
-                      if (dimValues == null || dimValues.isEmpty()) {
-                        dimSet.add(null);
+                      if (longPartitionDimensions.contains(dim)) {
+                        dimSet.add(canonicalLongValue(row.getRaw(dim), dim));

Review Comment:
   Fixed so that long dims now skip the string/range channel and prune only via the type-gated possibleInValueDomain.



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "jaykanakiya (via GitHub)" <gi...@apache.org>.
jaykanakiya commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3781932950


##########
processing/src/main/java/org/apache/druid/query/filter/TypedInFilter.java:
##########
@@ -277,6 +279,27 @@ public RangeSet<String> getDimensionRangeSet(String dimension)
     return retSet;
   }
 
+  @Nullable
+  @Override
+  public TypedValueSet getDimensionValueSet(String dimension)
+  {
+    if (!Objects.equals(getColumn(), dimension)) {
+      return null;
+    }
+    // Only LONG is safe (identical stringification on ingest and query); DOUBLE/FLOAT are excluded and STRING is
+    // handled by getDimensionRangeSet.
+    if (!matchValueType.is(ValueType.LONG)) {
+      return null;
+    }
+    // sortedMatchValues is coerced to matchValueType, so Evals.asString yields the same canonical strings as ingest.
+    // A null element is kept as a null set element so it matches a shard's stamped null (missing value).
+    final Set<String> values = new HashSet<>();
+    for (Object value : sortedMatchValues.get()) {
+      values.add(Evals.asString(value));

Review Comment:
   good catch, fixed by coercing before stringifying. 



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3830380575


##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/StreamingPartitionsSpec.java:
##########
@@ -55,7 +56,21 @@ public interface StreamingPartitionsSpec
    * segment's shard spec with it at publish time. Returns {@code null} when this spec is configured such that there is
    * nothing to collect (e.g. no dimensions), in which case no collector runs and segments are published unchanged.
    * One collector is created per task run.
+   *
+   * @param dimensionsSpec the task's declared dimensions, or {@code null} if none; lets a strategy consult per-dimension
+   *                       schema (e.g. to enable type-gated numeric pruning for {@code LONG} dimensions).
    */
   @Nullable
-  StreamingShardSpecCollector createCollector();
+  StreamingShardSpecCollector createCollector(@Nullable DimensionsSpec dimensionsSpec);

Review Comment:
   [P2] New abstract method breaks existing implementations
   
   Adding createCollector(DimensionsSpec) as a new abstract interface method breaks existing custom StreamingPartitionsSpec implementations: they no longer compile, and already-compiled implementations can throw AbstractMethodError when SeekableStreamIndexTaskRunner invokes the new method. The default no-arg bridge only preserves callers, not implementors. Make the new overload a default that delegates to createCollector(), or otherwise retain a default compatibility path while allowing the built-in spec to use DimensionsSpec.



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3614170348


##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java:
##########
@@ -747,13 +755,17 @@ public void run()
                           dim,
                           k -> Collections.synchronizedSet(new HashSet<>())
                       );
-                      // Empty getDimension result means a null/missing value; record null so IS NULL is not pruned
-                      // (distinct from "", which getDimension returns as ["" ]).
-                      final List<String> dimValues = row.getDimension(dim);
-                      if (dimValues == null || dimValues.isEmpty()) {
-                        dimSet.add(null);
+                      if (longPartitionDimensions.contains(dim)) {
+                        dimSet.add(canonicalLongValue(row.getRaw(dim), dim));

Review Comment:
   [P1] Disable legacy string pruning for stamped LONG dimensions
   
   Canonicalizing raw `"00001"` or `"+1"` to `"1"` is correct for the new typed domain, but the dimension still passes through `possibleInDomain` first. A native `SelectorDimFilter("code", "00001", ...)` matches indexed LONG `1` through runtime numeric coercion, while its range set remains literal `"00001"`. The range check therefore sees no overlap with stamped `"1"` and prunes the segment before typed pruning runs, dropping valid rows. Exclude type-stamped LONG dimensions from the legacy string/range channel, or safely canonicalize that channel, and add a regression test for noncanonical string selectors.



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "jaykanakiya (via GitHub)" <gi...@apache.org>.
jaykanakiya commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3810672960


##########
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:
   Updated. 



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3720545205


##########
processing/src/main/java/org/apache/druid/timeline/partition/DimensionValueSetShardSpec.java:
##########
@@ -89,6 +125,11 @@ public boolean possibleInDomain(Map<String, RangeSet<String>> domain)
       final String dimension = entry.getKey();
       final List<String> allowedValues = entry.getValue();
 
+      if (dimensionColumnTypes.containsKey(dimension)) {

Review Comment:
   [P2] Keep null pruning for stamped numeric dimensions
   
   LONG-stamped dimensions unconditionally skip `possibleInDomain`, but `NullFilter` only supplies that range domain and does not implement `getDimensionValueSet`. Consequently, `IS NULL` retains every LONG segment, including segments whose stamped values contain no null, despite this PR's numeric null-pruning coverage. Preserve null-only range pruning for stamped dimensions or add a typed null-domain representation and a regression test.



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "jaykanakiya (via GitHub)" <gi...@apache.org>.
jaykanakiya commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3772644037


##########
processing/src/main/java/org/apache/druid/timeline/partition/DimensionValueSetShardSpec.java:
##########
@@ -89,6 +125,11 @@ public boolean possibleInDomain(Map<String, RangeSet<String>> domain)
       final String dimension = entry.getKey();
       final List<String> allowedValues = entry.getValue();
 
+      if (dimensionColumnTypes.containsKey(dimension)) {

Review Comment:
   Addressed in the next commit. 



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3783671924


##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/StreamingPartitionsSpec.java:
##########
@@ -55,7 +56,10 @@ public interface StreamingPartitionsSpec
    * segment's shard spec with it at publish time. Returns {@code null} when this spec is configured such that there is
    * nothing to collect (e.g. no dimensions), in which case no collector runs and segments are published unchanged.
    * One collector is created per task run.
+   *
+   * @param dimensionsSpec the task's declared dimensions, or {@code null} if none; lets a strategy consult per-dimension
+   *                       schema (e.g. to enable type-gated numeric pruning for {@code LONG} dimensions).
    */
   @Nullable
-  StreamingShardSpecCollector createCollector();
+  StreamingShardSpecCollector createCollector(@Nullable DimensionsSpec dimensionsSpec);

Review Comment:
   [P2] Preserve collector-factory compatibility
   
   This replaces the existing public/pluggable `createCollector()` method with an abstract parameterized method. External implementations of `StreamingPartitionsSpec` will then fail to compile, or can throw `AbstractMethodError` when the task runner invokes the new method. Retain a compatibility bridge for existing implementations while adding the parameterized path.



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3775307888


##########
processing/src/main/java/org/apache/druid/query/filter/TypedInFilter.java:
##########
@@ -277,6 +279,27 @@ public RangeSet<String> getDimensionRangeSet(String dimension)
     return retSet;
   }
 
+  @Nullable
+  @Override
+  public TypedValueSet getDimensionValueSet(String dimension)
+  {
+    if (!Objects.equals(getColumn(), dimension)) {
+      return null;
+    }
+    // Only LONG is safe (identical stringification on ingest and query); DOUBLE/FLOAT are excluded and STRING is
+    // handled by getDimensionRangeSet.
+    if (!matchValueType.is(ValueType.LONG)) {
+      return null;
+    }
+    // sortedMatchValues is coerced to matchValueType, so Evals.asString yields the same canonical strings as ingest.
+    // A null element is kept as a null set element so it matches a shard's stamped null (missing value).
+    final Set<String> values = new HashSet<>();
+    for (Object value : sortedMatchValues.get()) {
+      values.add(Evals.asString(value));

Review Comment:
   [P2] Canonicalize presorted LONG IN values
   
   When `matchValueType=LONG` and `sortedMatchValues` contains a numeric value such as `1.0`, this loop stores `"1.0"`, while the existing LONG matcher coerces the same input to `1`. A stamped segment recording `"1"` can then be incorrectly pruned by typed segment pruning. Normalize LONG values with the existing LONG coercion before constructing `TypedValueSet`, and add a regression test.



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3614000139


##########
processing/src/main/java/org/apache/druid/timeline/partition/TypedValueSet.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.timeline.partition;
+
+import org.apache.druid.segment.column.ColumnType;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * A type-carrying set of stringified match values, produced by
+ * {@link org.apache.druid.query.filter.DimFilter#getDimensionValueSet(String)} and consumed by
+ * {@link ShardSpec#possibleInValueDomain(java.util.Map)}.
+ *
+ * <p>The value-domain analog of the {@code RangeSet<String>} used by
+ * {@link org.apache.druid.query.filter.DimFilter#getDimensionRangeSet(String)} /
+ * {@link ShardSpec#possibleInDomain(java.util.Map)}, but it also carries the filter's match {@link ColumnType}. That
+ * type is the safety gate for numeric segment pruning: a {@link DimensionValueSetShardSpec} may only prune when the
+ * filter's match type equals the type stamped for the dimension, because value stringification only agrees across
+ * ingest and query for identical types (LONG {@code 1} → {@code "1"}, but DOUBLE {@code 1.0} → {@code "1.0"}). A
+ * typeless {@code RangeSet<String>} cannot express this gate, which is why this is a separate channel.
+ *
+ * <p>The contained {@link #values} are the stringified match values (via {@code Evals.asString}), matching the
+ * ingest side. The set may contain a {@code null} element to denote a null/missing match value (e.g. an {@code IN}
+ * list containing {@code NULL}).
+ */
+public class TypedValueSet
+{
+  private final Set<String> values;
+  private final ColumnType type;
+
+  public TypedValueSet(Set<String> values, ColumnType type)
+  {
+    // Defensive copy into a set that tolerates a null element (denoting a null match value).
+    this.values = Collections.unmodifiableSet(new HashSet<>(values));
+    this.type = type;
+  }
+
+  public Set<String> getValues()

Review Comment:
   ## CodeQL / Exposing internal representation
   
   getValues exposes the internal representation stored in field values. The value may be modified [after this call to getValues](1).
   
   [Show more details](https://github.com/apache/druid/security/code-scanning/11349)



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #19704:
URL: https://github.com/apache/druid/pull/19704#issuecomment-6305466187

   This pull request has been marked as stale due to 60 days of inactivity.
   It will be closed in 4 weeks if no further activity occurs. If you think
   that's incorrect or this pull request should instead be reviewed, please simply
   write any comment. Even if closed, you can still revive the PR at any time or
   discuss it on the dev@druid.apache.org list.
   Thank you for your contributions.


-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
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: 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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "jaykanakiya (via GitHub)" <gi...@apache.org>.
jaykanakiya commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3810669041


##########
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:
   Good catch on the null path. I added testPruneDimValueSetIsNull, which covers the genuinely reachable null path end-to-end through the real FilterSegmentPruner: IS NULL on a LONG-stamped segment prunes one that observed no null and keeps one that did.



-- 
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

Re: [PR] feat: numeric pruning for dim_value_set segments (druid)

Posted by "jaykanakiya (via GitHub)" <gi...@apache.org>.
jaykanakiya commented on code in PR #19704:
URL: https://github.com/apache/druid/pull/19704#discussion_r3810670963


##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/DimensionValueSetCollector.java:
##########


Review Comment:
   Updated the doc. 



##########
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:
   Updated in next commit. 



-- 
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