Fix for schema mismatch to go down using the non vectorize path till we update the vectorized aggs properly by somu-imply · Pull Request #14924 · apache/druid

somu-imply · 2023-08-29T20:34:16Z

…we update the vectorized aggs properly

If in vectorized:force mode on a column which has segment which is a String column like

{
                "typeSignature": "STRING",
                "type": "STRING",
                "hasMultipleValues": false,
                "hasNulls": false,
                "size": 0,
                "cardinality": 1,
                "minValue": "0",
                "maxValue": "0",
                "errorMessage": null
            }

We run queries with numeric aggregation on that string column such as

"aggregations": [
   {
     "type": "floatLast",
     "name": "a0",
     "fieldName": "<string_col>",
     "timeColumn": "__time"
   }
 ],

the queries fail with an error

Error: RUNTIME_FAILURE (OPERATOR)

Cannot make VectorValueSelector for column with class[org.apache.druid.segment.column.StringUtf8DictionaryEncodedColumn]

org.apache.druid.java.util.common.UOE

This is because vectorized aggs need these cast supports. Till that is done, this PR fixes the issue by going down the non-vectorized route

…we update the vectorized aggs properly

clintropolis · 2023-08-30T05:51:40Z

...ssing/src/main/java/org/apache/druid/query/aggregation/last/DoubleLastAggregatorFactory.java

+    final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
+    return capabilities != null && capabilities.isNumeric();


i know i advised differently offline earlier, but i now think this can still return true, since the value selector creation is moved inside of the check in factorizeVector. looking closer at factorize and factorizeBuffer those will sort of end up using a 'nil' selector because the non-vectorize column value selector on these numeric aggs will call getDouble or getLong on the value selectors which will be null/zero for string inputs.

I did the same initially, but the get of NumericNilVectorAggregator would only return a null and not a 0 as the non-vectorized parts do. One solution would be to update the NumercNilVectorAggregator to return a serializablePair with the rhs as 0 on case of a null, but I am unsure if that will break anything else. I'll try to go down that path too

clintropolis · 2023-08-30T05:51:50Z

...essing/src/main/java/org/apache/druid/query/aggregation/last/FloatLastAggregatorFactory.java

+    final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
+    return capabilities != null && capabilities.isNumeric();


same comment about returning true

clintropolis · 2023-08-30T05:52:00Z

processing/src/main/java/org/apache/druid/query/aggregation/last/LongLastAggregatorFactory.java

+    final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
+    return capabilities != null && capabilities.isNumeric();


same comment about returning true

clintropolis · 2023-08-30T05:55:01Z

...ssing/src/main/java/org/apache/druid/query/aggregation/last/StringLastAggregatorFactory.java

+    final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
+    return capabilities != null && capabilities.getType().equals(ValueType.STRING);


this one probably is necessary to keep though, the string vector aggregator calls DimensionHandlerUtils.convertObjectToString which can handle any type of value, however it is using a VectorObjectSelector which isn't guaranteed to be implemented for numbers, which typically need to use VectorValueSelector.

…match_fix

somu-imply · 2023-09-08T16:59:25Z

Added the casting from valueSelector to ObjectSelector for non-string columns when used with stringLast/First in vectorized mode. Currently this fails on coverage only, adding tests soon

…ast on non string columns

…match_fix

clintropolis · 2023-09-12T19:38:48Z

processing/src/main/java/org/apache/druid/query/aggregation/any/NumericNilVectorAggregator.java

  @Nullable
  private final Object returnValue;

+  public static NumericNilVectorAggregator of(Object returnValue)


side note: I wonder if we should rename NumericNilVectorAggregator if we are going to be using it for non-numbers, maybe just NilVectorAggregator. I guess this doesn't need to be done in this PR, but it does seem kind of funny exposing it for other purposes

clintropolis · 2023-09-12T19:39:20Z

...ing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java

      return new DoubleFirstVectorAggregator(timeSelector, valueSelector);
    }
-    return NumericNilVectorAggregator.doubleNilVectorAggregator();
+    return NumericNilVectorAggregator.of(new SerializablePair<>(0L, NullHandling.defaultDoubleValue()));


this could be saved as a static variable instead of making a new one each time since its not going to change over the lifetime of the service, same for all the other impls

…ing static variables now

somu-imply · 2023-09-12T21:50:52Z

Addressed the review comments

Fix for schema mismatch to go down using the non vectorize path till …

fdeafdf

…we update the vectorized aggs properly

soumyava added Bug Area - Querying labels Aug 29, 2023

Fixing a failed test

f973f9e

clintropolis reviewed Aug 30, 2023

View reviewed changes

somu-imply added 5 commits September 6, 2023 11:31

Merge remote-tracking branch 'upstream/master' into latest_schema_mis…

f3ddd34

…match_fix

Updating numericNilAgg

635b389

Moving to use default values in case of nil agg

3bd6200

Adding the same for first agg

e98262b

Fixing a test

e4dafd3

somu-imply requested a review from clintropolis September 7, 2023 21:03

fixing vectorized string agg for last/first with cast if numeric

9434788

Updating tests to remove mockito and cover the case of string first/l…

b24dabf

…ast on non string columns

github-actions bot added Area - Ingestion and removed Area - Querying labels Sep 9, 2023

somu-imply added 2 commits September 8, 2023 18:15

Merge remote-tracking branch 'upstream/master' into latest_schema_mis…

a1ebb43

…match_fix

Updating a test to vectorize

68836a0

github-actions bot added the Area - Querying label Sep 9, 2023

somu-imply mentioned this pull request Sep 9, 2023

use VectorValueSelector instead of BaseLongVectorValueSelector for StringFirstAggregatorFactory.factorizeVector #14957

Merged

clintropolis reviewed Sep 12, 2023

View reviewed changes

Merge branch 'master' into latest_schema_mismatch_fix

ade66e5

github-actions bot removed the Area - Ingestion label Sep 12, 2023

Addressing review comments: Name change to NilVectorAggregator and us…

883b521

…ing static variables now

fixing intellij inspections

d0e0040

abhishekagarwal87 changed the title ~~Fix for schema mismatch to go down using the non vectorize path till …~~ Fix for schema mismatch to go down using the non vectorize path till we update the vectorized aggs properly Sep 13, 2023

clintropolis approved these changes Sep 13, 2023

View reviewed changes

soumyava merged commit bf99d2c into apache:master Sep 13, 2023

LakshSingla added this to the 28.0 milestone Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for schema mismatch to go down using the non vectorize path till we update the vectorized aggs properly#14924

Fix for schema mismatch to go down using the non vectorize path till we update the vectorized aggs properly#14924
soumyava merged 14 commits intoapache:masterfrom
somu-imply:latest_schema_mismatch_fix

somu-imply commented Aug 29, 2023 •

edited

Loading

Uh oh!

clintropolis Aug 30, 2023

Uh oh!

somu-imply Sep 5, 2023 •

edited

Loading

Uh oh!

clintropolis Aug 30, 2023

Uh oh!

clintropolis Aug 30, 2023

Uh oh!

clintropolis Aug 30, 2023

Uh oh!

somu-imply commented Sep 8, 2023

Uh oh!

clintropolis Sep 12, 2023

Uh oh!

clintropolis Sep 12, 2023

Uh oh!

somu-imply commented Sep 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
		return capabilities != null && capabilities.isNumeric();

Conversation

somu-imply commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintropolis Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

somu-imply Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

clintropolis Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

clintropolis Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

somu-imply commented Sep 8, 2023

Uh oh!

clintropolis Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

clintropolis Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

somu-imply commented Sep 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

somu-imply commented Aug 29, 2023 •

edited

Loading

somu-imply Sep 5, 2023 •

edited

Loading