Enabling aggregateMultipleValues in all StringAnyAggregators #15434

pranavbhole · 2023-11-27T07:49:16Z

Description

In case of MVDs, we need to decide if need to aggregate multiple values or pick up first one from the list. This PR enables aggregateMultipleValues optional flag in all StringAnyAggregators. Also exposed the flag in ANY_VALUE sql function.
Nuked Mockitos from StringAnyAggregator Factory test.

Fixed the bug ...

StringAnyVectorAggregator has some bug, in case of mvds, it always used to pick up first value, and it was inconsistent behavior with other StringAny Aggregator. aggregateMultipleValues is set to true by default. If user need first encountered values from mvd fields then aggregateMultipleValues can be set to false.

Release note

Adding aggregateMultipleValues optional flag in ANY_VALUE and StringAny aggregators.

This PR has:

...sing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java

clintropolis · 2023-11-27T23:35:43Z

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

 public class StringAnyAggregator implements Aggregator
 {
  private final BaseObjectColumnValueSelector valueSelector;
  private final int maxStringBytes;
  private boolean isFound;
  private String foundValue;
+  final boolean aggregateMultipleValues;


nit: private

clintropolis · 2023-11-27T23:38:36Z

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

@@ -49,12 +49,14 @@ public class StringAnyAggregatorFactory extends AggregatorFactory
  private final String fieldName;
  private final String name;
  protected final int maxStringBytes;
+  protected final boolean aggregateMultipleValues;


why protected? (same applies to maxStringBytes too i think...)

clintropolis · 2023-11-27T23:40:58Z

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

+            if (row.size() > 1) {
+              List<String> arrayList = new ArrayList<>();
+              row.forEach(rowIndex -> {
+                String chopped = StringUtils.fastLooseChop(multiValueSelector.lookupName(rowIndex), maxStringBytes);


i don't think you need to call fastLooseChop here because putValue uses StringUtils.toUtf8WithLimit which achieves the same thing

I removed it. Applied to arrays initially to avoid partially chopped result like [first, seco. applying chop at last leaves the result in weird open ended array. I think it might be expected as user can change it with increasing max bytes.

that's fine i think, i don't really like the silent truncation at all personally; i think we should probably change first/last/any to all optionally fail instead of silently truncating results, but that doesn't need to changed now.

clintropolis · 2023-11-27T23:46:30Z

...ing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactoryTest.java

+    columnSelectorFactory.moveSelectorCursorToNext();
+    columnSelectorFactory.moveSelectorCursorToNext();
+    res.aggregate();


why does it increment cursor twice before aggregating?

oh yes, I should have probably mention it in comment, Dummy TestColumnSelectorFactory Selector that I wrote at the end of class has MVD present on 3rd place, this moved cursor twice.

clintropolis · 2023-11-27T23:47:57Z

...in/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java

+          case COMPLEX:
+            return new StringAnyAggregatorFactory(name, fieldName, maxStringBytes, aggregateMultipleValues);


why would complex map to string? I think this means any type of complex would use the string aggregator which doesn't seem correct to me

clintropolis · 2023-11-27T23:50:18Z

...in/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java

+  private Integer getMaxStringBytes(
+      final List<RexNode> rexNodes,
+      final AggregateCall aggregateCall,
+      final PlannerContext plannerContext


this method seems strange to pass List<RexNode>, and only uses rexNodes.get(1) from it, please pass the RexNode to check directly

pranavbhole · 2023-11-28T02:38:03Z

Addressed all comments.

pranavbhole · 2023-11-28T04:45:39Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

+        .sql("SELECT ANY_VALUE(dim3, 1000, null) FROM foo")
+        .queryContext(ImmutableMap.of());
+
+    DruidException e1 = assertThrows(DruidException.class, () -> qtb1.run());


@kgyrtkirk Looks like we are getting unclear error message in decoupled mode than coupled mode. I had to had condition for asserts.

from Query could not be planned. A possible reason is
it seem like it went fishing - but the error is [The third argument 'null:NULL' to function 'EXPR$0' is not a boolean] which should be identified much earlier.

I believe the best way to fix this inconsistency would be to make sure this is checked during validation.

Another way around is to use @NotYetSupported and leave it for later....

added early validation in operandChecker class and removed the number/boolean checks.

clintropolis

overall lgtm

clintropolis · 2023-11-28T05:34:11Z

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

+              });
+              foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+            } else {
+              foundValue = multiValueSelector.lookupName(row.get(0));


this doesn't seem to exactly match the behavior of the StringAnyAggregator and StringAnyBufferAggregator, which only have special case handling for 0 length lists. I think this is probably 'better' behavior for mvds since it matches the behavior of scan queries which only show mvds as an array if the length is bigger than 1, should we modify the other two implementations to be consistent for handling the case where the mvd row only has a single value?

made it consistent across all aggregator to return single value in case of array size 1

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

processing/src/test/java/org/apache/druid/query/aggregation/AggregatorFactoryTest.java

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

...sing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java

docs/querying/sql-functions.md

kgyrtkirk · 2023-11-28T05:23:40Z

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

+        List<Object> objectList = (List) object;
+        foundValue = objectList.size() > 0 ? DimensionHandlerUtils.convertObjectToString(objectList.get(0)) : null;


shouldn't this go over all elements of the list? the vector implementation seems to be doing that

nope, we check the flag aggregateMultipleValues and return all elements stringified

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

kgyrtkirk · 2023-11-28T05:29:32Z

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

+              });
+              foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+            } else {
+              foundValue = multiValueSelector.lookupName(row.get(0));


the other impelementation called DimensionHandlerUtils.convertObjectToString even on a single value - is that ok; shouldn't they be in sync?

DimensionHandlerUtils.convertObjectToString handles null value and here foundValue null is also handled.

kgyrtkirk · 2023-11-28T05:51:07Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

@@ -13433,6 +13433,45 @@ public void testStringAggExpressionNonConstantSeparator()
    );
  }

+  @Test
+  public void testStringAnyAggArgValidation()


there seems to be multiple testcases in this test; can you separate them?

kgyrtkirk · 2023-11-28T05:55:12Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

+        .sql("SELECT ANY_VALUE(dim3, 1000, null) FROM foo")
+        .queryContext(ImmutableMap.of());
+
+    DruidException e1 = assertThrows(DruidException.class, () -> qtb1.run());


from Query could not be planned. A possible reason is
it seem like it went fishing - but the error is [The third argument 'null:NULL' to function 'EXPR$0' is not a boolean] which should be identified much earlier.

I believe the best way to fix this inconsistency would be to make sure this is checked during validation.

Another way around is to use @NotYetSupported and leave it for later....

kgyrtkirk · 2023-11-28T06:02:41Z

...in/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java

+        );
+        break;
+      case 3:
+        maxStringBytes = getMaxStringBytes(rexNodes.get(1), aggregateCall, plannerContext);


nit: copy-paste - as the createAggregatorFactory will be called anyway with a null if its not specified; wouldn't it be better to just keep a variable and set its value when available?

kgyrtkirk · 2023-11-28T06:08:24Z

...in/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java

          plannerContext.setPlanningError(
-              "The second argument '%s' to function '%s' is not a number",
-              rexNodes.get(1),
+              "The third argument '%s' to function '%s' is not a boolean",
+              rexNodes.get(2),
              aggregateCall.getName()
          );


this is a bit too late... a check like this should happen during validation and not during aggregator production; a straight DruidException might also be better.

removed the code as early validation is added

...in/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java

-          );
-          return null;
-        }
+        Integer maxStringBytes = RexLiteral.intValue(rexNodes.get(1)); // added not null check at the function


kgyrtkirk · 2023-11-29T06:23:54Z

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

+    }
+    if (object instanceof List) {
+      List<Object> objectList = (List) object;
+      if (objectList.size() == 1) {


can the size() == 0 ? if yes that could be a problem
note: I wonder if ifs can be flipped; since when aggregateMultipleValues is true all branches lead to the same decision; could something like this work?

if( !aggregateMultipleValues && (object instanceof List) && ((List)object).size() > 1 ) { return DimensionHandlerUtils.convertObjectToString(objectList.get(0)); } else { DimensionHandlerUtils.convertObjectToString(object); }

added size 0 check. It can be possible that we ingest empty arrays i guess.

docs/querying/sql-aggregations.md

clintropolis · 2023-11-29T07:57:56Z

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

+      if (objectList.size() == 1) {
+        return DimensionHandlerUtils.convertObjectToString(objectList.get(0));
+      }
+      if (aggregateMultipleValues) {
+        return DimensionHandlerUtils.convertObjectToString(objectList);
+      }
+      return DimensionHandlerUtils.convertObjectToString(objectList.get(0));


now that I think about it, for the non-vectorized engine, string object selectors actually already unwrap the list if it is only a single element, so I don't think we might not actually need special handling for list size 1
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/DimensionSelector.java#L141 which is called by https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/DimensionSelector.java#L128 which is what is used by the multi-value dimension selector when reading from segments https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/column/StringUtf8DictionaryEncodedColumn.java#L229 (no other dimension selectors for mvds override this behavior)

sorry for the confusion, the value will always be a String or null in the case of size 1 or size 0 when using getObject().

@clintropolis Thank you Clint. Is it ok to leave this extra safety check in the code ?

if this was ... any (heh!) other aggregator i would say change it (and probably re-arrange the if) because aggregate is in a hot path, but since this one basically only calls aggregate once and then short-circuits the rest of the time... so its probably fine

Co-authored-by: Clint Wylie <cjwylie@gmail.com>

Updating the native docs for #15434

…15434) * Enabling aggregateMultipleValues in all StringAnyAggregators * Adding more tests * More validation * fix warning * updating asserts in decoupled mode * fix intellij inspection * Addressing comments * Addressing comments * Adding early validations and make aggregate consistent across all * fixing tests * fixing tests * Update docs/querying/sql-aggregations.md Co-authored-by: Clint Wylie <cjwylie@gmail.com> * fixing static check --------- Co-authored-by: Clint Wylie <cjwylie@gmail.com>

Updating the native docs for apache#15434

…15434) * Enabling aggregateMultipleValues in all StringAnyAggregators * Adding more tests * More validation * fix warning * updating asserts in decoupled mode * fix intellij inspection * Addressing comments * Addressing comments * Adding early validations and make aggregate consistent across all * fixing tests * fixing tests * Update docs/querying/sql-aggregations.md Co-authored-by: Clint Wylie <cjwylie@gmail.com> * fixing static check --------- Co-authored-by: Clint Wylie <cjwylie@gmail.com>

Updating the native docs for apache#15434

Enabling aggregateMultipleValues in all StringAnyAggregators

9428c6b

github-actions bot added Area - Documentation Area - Querying labels Nov 27, 2023

github-advanced-security bot found potential problems Nov 27, 2023

View reviewed changes

...sing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java Fixed Show fixed Hide fixed

...sing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java Fixed Show fixed Hide fixed

pranavbhole added 4 commits November 27, 2023 11:21

Adding more tests

09c3e87

More validation

360b3f5

fix warning

11d1820

updating asserts in decoupled mode

d741ed4

317brian added the Release Notes label Nov 27, 2023

clintropolis reviewed Nov 27, 2023

View reviewed changes

pranavbhole added 3 commits November 27, 2023 16:42

fix intellij inspection

69a9c4a

Addressing comments

0d47550

Addressing comments

31be586

pranavbhole commented Nov 28, 2023

View reviewed changes

clintropolis reviewed Nov 28, 2023

View reviewed changes

soumyava reviewed Nov 28, 2023

View reviewed changes

kgyrtkirk reviewed Nov 28, 2023

View reviewed changes

Adding early validations and make aggregate consistent across all

8771ad3

github-advanced-security bot found potential problems Nov 29, 2023

View reviewed changes

kgyrtkirk reviewed Nov 29, 2023

View reviewed changes

pranavbhole added 2 commits November 28, 2023 23:43

fixing tests

7b3d654

fixing tests

4be07da

clintropolis reviewed Nov 29, 2023

View reviewed changes

pranavbhole and others added 2 commits November 29, 2023 09:02

Update docs/querying/sql-aggregations.md

651b634

Co-authored-by: Clint Wylie <cjwylie@gmail.com>

fixing static check

cd74095

clintropolis approved these changes Nov 29, 2023

View reviewed changes

soumyava approved these changes Nov 29, 2023

View reviewed changes

soumyava merged commit 93cd638 into apache:master Nov 29, 2023
83 checks passed

pranavbhole mentioned this pull request Nov 29, 2023

Native doc update #15456

Merged

10 tasks

pranavbhole deleted the aggregateMultipleValues branch November 29, 2023 22:59

abhishekagarwal87 pushed a commit that referenced this pull request Nov 30, 2023

Native doc update (#15456)

74ab602

Updating the native docs for #15434

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

Native doc update (apache#15456)

04741a4

Updating the native docs for apache#15434

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

Native doc update (apache#15456)

fd0c5da

Updating the native docs for apache#15434

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

Enabling aggregateMultipleValues in all StringAnyAggregators #15434

Enabling aggregateMultipleValues in all StringAnyAggregators #15434

pranavbhole commented Nov 27, 2023 •

edited

clintropolis Nov 27, 2023

pranavbhole Nov 28, 2023

clintropolis Nov 27, 2023

clintropolis Nov 27, 2023

pranavbhole Nov 28, 2023

clintropolis Nov 28, 2023

clintropolis Nov 27, 2023

pranavbhole Nov 28, 2023

clintropolis Nov 27, 2023

clintropolis Nov 27, 2023

pranavbhole commented Nov 28, 2023

pranavbhole Nov 28, 2023

kgyrtkirk Nov 28, 2023

pranavbhole Nov 29, 2023

clintropolis left a comment

clintropolis Nov 28, 2023

pranavbhole Nov 29, 2023

kgyrtkirk Nov 28, 2023

pranavbhole Nov 29, 2023

kgyrtkirk Nov 28, 2023

pranavbhole Nov 29, 2023

kgyrtkirk Nov 28, 2023

kgyrtkirk Nov 28, 2023

kgyrtkirk Nov 28, 2023

kgyrtkirk Nov 28, 2023

pranavbhole Nov 29, 2023

kgyrtkirk Nov 29, 2023

pranavbhole Nov 29, 2023

clintropolis Nov 29, 2023

pranavbhole Nov 29, 2023

clintropolis Nov 29, 2023

		case COMPLEX:
		return new StringAnyAggregatorFactory(name, fieldName, maxStringBytes, aggregateMultipleValues);

		List<Object> objectList = (List) object;
		foundValue = objectList.size() > 0 ? DimensionHandlerUtils.convertObjectToString(objectList.get(0)) : null;

Enabling aggregateMultipleValues in all StringAnyAggregators #15434

Enabling aggregateMultipleValues in all StringAnyAggregators #15434

Conversation

pranavbhole commented Nov 27, 2023 • edited

Description

Fixed the bug ...

Release note

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

Choose a reason for hiding this comment

pranavbhole commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis 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

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

pranavbhole commented Nov 27, 2023 •

edited