add native 'array contains element' filter #15366

clintropolis · 2023-11-14T05:54:35Z

Description

Alternative mentioned in #15350, which is simplified and only matches a single array element rather than being a full implementation of 'ARRAY_CONTAINS'. Decided to do this one when I realized that multi-value dimensions would be handled incorrectly with the other filter when using array type element matches.

Array element indexes are already hiding inside of array type columns, this PR adds a native arrayContainsElement filter so we can actually use them to implement things like ARRAY_CONTAINS (in this PR) and ARRAY_OVERLAP (not in this PR).

No documentation currently added, will do that in a follow-up PR.

As expected, this is a giant perf improvement when using ARRAY_CONTAINS on array columns. For the query added in the benchmarks:

SELECT string1, long1 FROM foo WHERE ARRAY_CONTAINS("multi-string3", 100) GROUP BY 1,2

Before:

Benchmark                        (query)  (rowsPerSegment)  (schema)  (vectorize)  Mode  Cnt     Score    Error  Units
SqlExpressionBenchmark.querySql       38           5000000      auto        false  avgt    5  4839.508 ± 89.496  ms/op

After:

Benchmark                        (query)  (rowsPerSegment)  (schema)  (vectorize)  Mode  Cnt    Score   Error  Units
SqlExpressionBenchmark.querySql       38           5000000      auto        false  avgt    5  133.610 ± 3.750  ms/op

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

soumyava

Took one pass. LGTM so far. Will run locally today and do a 2nd pass

soumyava · 2023-11-17T20:21:10Z

benchmarks/src/test/java/org/apache/druid/benchmark/query/SqlBenchmark.java

@@ -413,7 +413,24 @@ public class SqlBenchmark
      "SELECT APPROX_COUNT_DISTINCT_BUILTIN(dimZipf) FROM foo",
      "SELECT APPROX_COUNT_DISTINCT_DS_HLL(dimZipf) FROM foo",
      "SELECT APPROX_COUNT_DISTINCT_DS_HLL_UTF8(dimZipf) FROM foo",
-      "SELECT APPROX_COUNT_DISTINCT_DS_THETA(dimZipf) FROM foo"
+      "SELECT APPROX_COUNT_DISTINCT_DS_THETA(dimZipf) FROM foo",
+      // 32: LATEST aggregator long


Thanks for moving these from expression benchmarks to here

abhishekagarwal87

Overall seems good. Though tbh, the whole matchers and processors classes are a maze of abstractions that I find difficult to navigate through. So I might have missed a thing here and there.

abhishekagarwal87 · 2023-11-28T10:18:06Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

-      ExpressionType type = ExpressionType.LONG;
+      ExpressionType type = null;
      for (Expr arg : args) {
-        type = ExpressionTypeConversion.function(type, arg.getOutputType(inspector));
+        type = ExpressionTypeConversion.leastRestrictiveType(type, arg.getOutputType(inspector));
      }
-      return ExpressionType.asArrayType(type);
+      return type == null ? null : ExpressionTypeFactory.getInstance().ofArray(type);
    }


why was this changed?

it was wrong in some cases and written before leastRestrictiveType existed, for example if you try to use the array constructor function on an array type input, you end up with

org.apache.druid.segment.column.Types$IncompatibleTypeException: Cannot implicitly cast [LONG] to [ARRAY<STRING>]

which doesn't happen after the change. Will add a test.

abhishekagarwal87 · 2023-11-28T10:19:01Z

processing/src/main/java/org/apache/druid/query/filter/DimFilter.java

@@ -53,7 +53,8 @@
    @JsonSubTypes.Type(name = "equals", value = EqualityFilter.class),
    @JsonSubTypes.Type(name = "range", value = RangeFilter.class),
    @JsonSubTypes.Type(name = "isfalse", value = IsFalseDimFilter.class),
-    @JsonSubTypes.Type(name = "istrue", value = IsTrueDimFilter.class)
+    @JsonSubTypes.Type(name = "istrue", value = IsTrueDimFilter.class),
+    @JsonSubTypes.Type(name = "arrayContainsElement", value = ArrayContainsElementFilter.class)


a shorter name such as arrayContains sounds better.

I didn't want to confuse this native filter with the array_contains function which this filter can be used to implement but doesn't completely implement (see PR description), so I'd prefer to leave it like this to avoid confusion

processing/src/main/java/org/apache/druid/query/filter/ArrayContainsElementFilter.java

abhishekagarwal87 · 2023-11-28T12:26:17Z

processing/src/main/java/org/apache/druid/query/filter/ArrayContainsElementFilter.java

+    }
+  }
+
+  private static class TypedConstantElementValueMatcherFactory extends EqualityFilter.TypedConstantValueMatcherFactory


some javadocs here would be useful.

abhishekagarwal87 · 2023-11-28T13:52:10Z

processing/src/main/java/org/apache/druid/query/filter/ArrayContainsElementFilter.java

+        this.doublePredicateSupplier = equalityPredicateFactory::makeDoublePredicate;
+        this.floatPredicateSupplier = equalityPredicateFactory::makeFloatPredicate;
+      }
+      this.objectPredicateSupplier = makeObjectPredicateSupplier();


this method results in a call stack that refers to typeDetectingArrayPredicateSupplier which is not initialized yet.
makeObjectPredicateSupplier -> makeArrayPredicate -> typeDetectingArrayPredicateSupplier

nothing will get these predicates until after the constructor though, what is the problem you're worried about?

abhishekagarwal87 · 2023-11-28T14:10:41Z

processing/src/main/java/org/apache/druid/query/filter/ArrayContainsElementFilter.java

+        @Nullable ColumnCapabilities columnCapabilities
+    )
+    {
+      return super.makeArrayProcessor(selector, columnCapabilities);


while it kinda does the right thing right now, it feels too tightly coupled with EqualityFilter. It confused me for some time till I realized that you are passing the predicateMatcherFactory to super class. The way I read it as that elementMatchValue can be compared to input array through "equals" comparison. I think it would be better to just call predicateMatcherFactory.makeArrayProcessor(selector, columnCapabilities); and in makeComplexProcessor

while it kinda does the right thing right now, it feels too tightly coupled with EqualityFilter

that is the point though, if the filter is matching a scalar column and the match value is a non-null scalar value then this filter should behave identical to equality filter, the ask during review of #15350 was to re-use as much of the equality filter as possible because in certain cases it is the equality filter

actually, i just removed these overrides since they were pointless

abhishekagarwal87 · 2023-11-28T14:28:43Z

btw how do the benchmark numbers look like if you disable bitmap indexes?

clintropolis · 2023-11-28T20:21:14Z

btw how do the benchmark numbers look like if you disable bitmap indexes?

like with filterTuning? I didn't check because SQL has no way to control filterTuning, I would guess somewhere between the expression filter numbers and index numbers, but can check in a bit

clintropolis · 2023-11-28T22:41:54Z

btw how do the benchmark numbers look like if you disable bitmap indexes?

ok re-ran benchmarks (my laptop a bit busier this time than last time so stuff a bit slower)

expression filter:

Benchmark                        (query)  (rowsPerSegment)  (schema)  (vectorize)  Mode  Cnt     Score    Error  Units
SqlExpressionBenchmark.querySql       38           5000000      auto        false  avgt    5  5135.236 ± 77.511  ms/op

arrayContainsElement filter (indexes commented out):

Benchmark                        (query)  (rowsPerSegment)  (schema)  (vectorize)  Mode  Cnt     Score     Error  Units
SqlExpressionBenchmark.querySql       38           5000000      auto        false  avgt    5  5140.199 ± 141.967  ms/op

arrayContainsElement filter (with indexes):

Benchmark                        (query)  (rowsPerSegment)  (schema)  (vectorize)  Mode  Cnt    Score   Error  Units
SqlExpressionBenchmark.querySql       38           5000000      auto        false  avgt    5  149.909 ± 2.783  ms/op

…mns (#15451) Updates ARRAY_OVERLAP to use the same ArrayContainsElement filter added in #15366 when filtering ARRAY typed columns so that it can also use indexes like ARRAY_CONTAINS.

* add native arrayContainsElement filter to use array column element indexes

…mns (apache#15451) Updates ARRAY_OVERLAP to use the same ArrayContainsElement filter added in apache#15366 when filtering ARRAY typed columns so that it can also use indexes like ARRAY_CONTAINS.

* add native arrayContainsElement filter to use array column element indexes

…mns (apache#15451) Updates ARRAY_OVERLAP to use the same ArrayContainsElement filter added in apache#15366 when filtering ARRAY typed columns so that it can also use indexes like ARRAY_CONTAINS.

clintropolis added 7 commits November 8, 2023 17:21

add native array_contains filter to use array column element indexes

f1588eb

fix array equality

f91dce6

rearrange

8db6bae

fix test and style

6c6be26

cleanup, more test, benchmark

0a2c08b

fix it

0cf0fff

rework into single element matching filter

72176f1

clintropolis added Performance Area - Querying Area - SQL labels Nov 14, 2023

github-actions bot added Area - Segment Format and Ser/De Area - Ingestion labels Nov 14, 2023

fix test

671806b

clintropolis mentioned this pull request Nov 15, 2023

add native array_contains filter to use array column element indexes #15350

Closed

10 tasks

soumyava reviewed Nov 17, 2023

View reviewed changes

abhishekagarwal87 reviewed Nov 28, 2023

View reviewed changes

cleanup

46d6856

abhishekagarwal87 approved these changes Nov 29, 2023

View reviewed changes

less sad

e34d380

clintropolis merged commit 64fcb32 into apache:master Nov 29, 2023
83 checks passed

clintropolis deleted the array-contains-element-filter branch November 29, 2023 11:33

This was referenced Nov 29, 2023

update ARRAY_OVERLAP to plan with ArrayContainsElement for ARRAY columns #15451

Merged

document arrayContainsElement filter #15455

Merged

yashdeep97 pushed a commit to yashdeep97/druid that referenced this pull request Dec 1, 2023

add native 'array contains element' filter (apache#15366)

951865a

* add native arrayContainsElement filter to use array column element indexes

yashdeep97 pushed a commit to yashdeep97/druid that referenced this pull request Dec 1, 2023

add native 'array contains element' filter (apache#15366)

b5532e0

* add native arrayContainsElement filter to use array column element indexes

LakshSingla added this to the 29.0.0 milestone Jan 29, 2024

LakshSingla mentioned this pull request Feb 13, 2024

[DRAFT] 29.0.0 release notes #15896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add native 'array contains element' filter #15366

add native 'array contains element' filter #15366

clintropolis commented Nov 14, 2023 •

edited

Loading

soumyava left a comment

soumyava Nov 17, 2023

abhishekagarwal87 left a comment

abhishekagarwal87 Nov 28, 2023

clintropolis Nov 28, 2023

abhishekagarwal87 Nov 28, 2023

clintropolis Nov 28, 2023

abhishekagarwal87 Nov 28, 2023

abhishekagarwal87 Nov 28, 2023

clintropolis Nov 28, 2023

abhishekagarwal87 Nov 28, 2023

clintropolis Nov 28, 2023 •

edited

Loading

clintropolis Nov 28, 2023

abhishekagarwal87 commented Nov 28, 2023

clintropolis commented Nov 28, 2023

clintropolis commented Nov 28, 2023

add native 'array contains element' filter #15366

add native 'array contains element' filter #15366

Conversation

clintropolis commented Nov 14, 2023 • edited Loading

Description

soumyava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekagarwal87 commented Nov 28, 2023

clintropolis commented Nov 28, 2023

clintropolis commented Nov 28, 2023

clintropolis commented Nov 14, 2023 •

edited

Loading

clintropolis Nov 28, 2023 •

edited

Loading