You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "pranavbhole (via GitHub)" <gi...@apache.org> on 2023/11/27 07:49:19 UTC
[PR] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
pranavbhole opened a new pull request, #15434:
URL: https://github.com/apache/druid/pull/15434
### 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.
#### 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.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] 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 or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] 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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408885801
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java:
##########
@@ -65,6 +67,27 @@ public void aggregate(ByteBuffer buf, int position)
}
}
+ private String readValue(Object object)
+ {
+ if (object == null) {
+ return null;
+ }
+ if (object instanceof List) {
+ List<Object> objectList = (List) object;
+ if (objectList.size() == 0) {
+ return null;
+ }
+ if (objectList.size() == 1) {
+ return DimensionHandlerUtils.convertObjectToString(objectList.get(0));
+ }
+ if (aggregateMultipleValues) {
+ return DimensionHandlerUtils.convertObjectToString(objectList);
+ }
+ return DimensionHandlerUtils.convertObjectToString(objectList.get(0));
Review Comment:
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().
##########
docs/querying/sql-aggregations.md:
##########
@@ -90,7 +90,7 @@ In the aggregation functions supported by Druid, only `COUNT`, `ARRAY_AGG`, and
|`EARLIEST_BY(expr, timestampExpr, [maxBytesPerValue])`|Returns the earliest value of `expr`.<br />The earliest value of `expr` is taken from the row with the overall earliest non-null value of `timestampExpr`. <br />If the earliest non-null value of `timestampExpr` appears in multiple rows, the `expr` may be taken from any of those rows.<br /><br />If `expr` is a string or complex type `maxBytesPerValue` amount of space is allocated for the aggregation. Strings longer than this limit are truncated. The `maxBytesPerValue` parameter should be set as low as possible, since high values will lead to wasted memory.<br/>If `maxBytesPerValue`is omitted; it defaults to `1024`. |`null` or `0`/`''` if `druid.generic.useDefaultValueForNull=true` (legacy mode)|
|`LATEST(expr, [maxBytesPerValue])`|Returns the latest value of `expr`<br />The `expr` must come from a relation with a timestamp column (like `__time` in a Druid datasource) and the "latest" is taken from the row with the overall latest non-null value of the timestamp column.<br />If the latest non-null value of the timestamp column appears in multiple rows, the `expr` may be taken from any of those rows.<br /><br />If `expr` is a string or complex type `maxBytesPerValue` amount of space is allocated for the aggregation. Strings longer than this limit are truncated. The `maxBytesPerValue` parameter should be set as low as possible, since high values will lead to wasted memory.<br/>If `maxBytesPerValue`is omitted; it defaults to `1024`. |`null` or `0`/`''` if `druid.generic.useDefaultValueForNull=true` (legacy mode)|
|`LATEST_BY(expr, timestampExpr, [maxBytesPerValue])`|Returns the latest value of `expr`.<br />The latest value of `expr` is taken from the row with the overall latest non-null value of `timestampExpr`.<br />If the overall latest non-null value of `timestampExpr` appears in multiple rows, the `expr` may be taken from any of those rows.<br /><br />If `expr` is a string or complex type `maxBytesPerValue` amount of space is allocated for the aggregation. Strings longer than this limit are truncated. The `maxBytesPerValue` parameter should be set as low as possible, since high values will lead to wasted memory.<br/>If `maxBytesPerValue`is omitted; it defaults to `1024`. |`null` or `0`/`''` if `druid.generic.useDefaultValueForNull=true` (legacy mode)|
-|`ANY_VALUE(expr, [maxBytesPerValue])`|Returns any value of `expr` including null. This aggregator can simplify and optimize the performance by returning the first encountered value (including `null`).<br /><br />If `expr` is a string or complex type `maxBytesPerValue` amount of space is allocated for the aggregation. Strings longer than this limit are truncated. The `maxBytesPerValue` parameter should be set as low as possible, since high values will lead to wasted memory.<br/>If `maxBytesPerValue`is omitted; it defaults to `1024`. |`null` or `0`/`''` if `druid.generic.useDefaultValueForNull=true` (legacy mode)|
+|`ANY_VALUE(expr, [maxBytesPerValue, [aggregateMultipleValues]])`|Returns any value of `expr` including null. This aggregator can simplify and optimize the performance by returning the first encountered value (including `null`).<br /><br />If `expr` is a string or complex type `maxBytesPerValue` amount of space is allocated for the aggregation. Strings longer than this limit are truncated. The `maxBytesPerValue` parameter should be set as low as possible, since high values will lead to wasted memory.<br/>If `maxBytesPerValue`is omitted; it defaults to `1024`. `aggregateMultipleValues` is optional boolean flag defines behavior of aggregating multiple values in field. `aggregateMultipleValues` is set as true by default and returns the array in case of Multiple values (MVD) in field. By setting it to false, function will return first encountered value in case of MVDs. |`null` or `0`/`''` if `druid.generic.useDefaultValueForNull=true` (legacy mode)|
Review Comment:
we should consider linking to https://github.com/apache/druid/blob/master/docs/querying/multi-value-dimensions.md here i think
```suggestion
|`ANY_VALUE(expr, [maxBytesPerValue, [aggregateMultipleValues]])`|Returns any value of `expr` including null. This aggregator can simplify and optimize the performance by returning the first encountered value (including `null`).<br /><br />If `expr` is a string or complex type `maxBytesPerValue` amount of space is allocated for the aggregation. Strings longer than this limit are truncated. The `maxBytesPerValue` parameter should be set as low as possible, since high values will lead to wasted memory.<br/>If `maxBytesPerValue` is omitted; it defaults to `1024`. `aggregateMultipleValues` is an optional boolean flag controls the behavior of aggregating a [multi-value dimension](./multi-value-dimensions.md). `aggregateMultipleValues` is set as true by default and returns the stringified array in case of a multi-value dimension. By setting it to false, function will return first value instead. |`null` or `0`/`''` if `druid.generic.useDefaultValueForNull=true` (legacy mode)|
```
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1407083766
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -13433,6 +13433,45 @@ public void testStringAggExpressionNonConstantSeparator()
);
}
+ @Test
+ public void testStringAnyAggArgValidation()
+ {
+ DruidException e = assertThrows(DruidException.class, () -> testBuilder()
+ .sql("SELECT ANY_VALUE(dim3, 1000, 'true') FROM foo")
+ .queryContext(ImmutableMap.of())
+ .run());
+ assertThat(
+ e,
+ invalidSqlIs(
+ "Cannot apply 'ANY_VALUE' to arguments of type 'ANY_VALUE(<VARCHAR>, <INTEGER>, <CHAR(4)>)'. Supported form(s): 'ANY_VALUE(<expr>, [<maxBytesPerString>, [<aggregateMultipleValues>]])' (line [1], column [8])")
+ );
+ QueryTestBuilder qtb1 = testBuilder()
+ .sql("SELECT ANY_VALUE(dim3, 1000, null) FROM foo")
+ .queryContext(ImmutableMap.of());
+
+ DruidException e1 = assertThrows(DruidException.class, () -> qtb1.run());
Review Comment:
@kgyrtkirk Looks like we are getting unclear error message in decoupled mode than coupled mode. I had to had condition for asserts.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1407192800
##########
docs/querying/sql-functions.md:
##########
@@ -50,7 +50,7 @@ Calculates the arc cosine of a numeric expression.
## ANY_VALUE
-`ANY_VALUE(expr, [maxBytesPerValue])`
+`ANY_VALUE(expr, [maxBytesPerValue, aggregateMultipleValues])`
Review Comment:
I believe `ANY_VALUE(expr, [maxBytesPerValue, aggregateMultipleValues])` means that the 2 way this function can be called is:
`ANY_VALUE(expr)` and `ANY_VALUE(expr, maxBytesPerValue, aggregateMultipleValues)`
shouldn't this be `ANY_VALUE(expr, [maxBytesPerValue, [aggregateMultipleValues]])` ?
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,22 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ arrayList.add(multiValueSelector.lookupName(rowIndex));
+ });
+ foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
+ }
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
+ }
Review Comment:
note: I beleive this could be moved into a method; so that every `foundValue` assignment could become a `return`
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -244,17 +249,35 @@ public Aggregation toDruidAggregation(
final AggregatorFactory theAggFactory;
switch (args.size()) {
case 1:
- theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null);
+ theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null, true);
break;
case 2:
- int maxStringBytes;
+ Integer maxStringBytes = getMaxStringBytes(rexNodes.get(1), aggregateCall, plannerContext);
+ if (maxStringBytes == null) {
+ return null;
+ }
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ null,
+ outputType,
+ maxStringBytes.intValue(),
+ true
+ );
+ break;
+ case 3:
+ maxStringBytes = getMaxStringBytes(rexNodes.get(1), aggregateCall, plannerContext);
+ if (maxStringBytes == null) {
+ return null;
+ }
+ boolean aggregateMultipleValues;
try {
- maxStringBytes = RexLiteral.intValue(rexNodes.get(1));
+ aggregateMultipleValues = RexLiteral.booleanValue(Preconditions.checkNotNull(rexNodes.get(2)));
}
- catch (AssertionError ae) {
+ catch (Exception ae) {
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()
);
Review Comment:
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.
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -244,17 +249,35 @@ public Aggregation toDruidAggregation(
final AggregatorFactory theAggFactory;
switch (args.size()) {
case 1:
- theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null);
+ theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null, true);
break;
case 2:
- int maxStringBytes;
+ Integer maxStringBytes = getMaxStringBytes(rexNodes.get(1), aggregateCall, plannerContext);
+ if (maxStringBytes == null) {
+ return null;
+ }
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ null,
+ outputType,
+ maxStringBytes.intValue(),
+ true
+ );
+ break;
+ case 3:
+ maxStringBytes = getMaxStringBytes(rexNodes.get(1), aggregateCall, plannerContext);
Review Comment:
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?
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java:
##########
@@ -52,7 +55,13 @@ public void aggregate(ByteBuffer buf, int position)
{
if (buf.getInt(position) == NOT_FOUND_FLAG_VALUE) {
final Object object = valueSelector.getObject();
- String foundValue = DimensionHandlerUtils.convertObjectToString(object);
+ String foundValue;
+ if (object != null && object instanceof List && !aggregateMultipleValues) {
+ List<Object> objectList = (List) object;
+ foundValue = objectList.size() > 0 ? DimensionHandlerUtils.convertObjectToString(objectList.get(0)) : null;
Review Comment:
shouldn't this go over all elements of the list? the vector implementation seems to be doing that
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -13433,6 +13433,45 @@ public void testStringAggExpressionNonConstantSeparator()
);
}
+ @Test
+ public void testStringAnyAggArgValidation()
Review Comment:
there seems to be multiple testcases in this test; can you separate them?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -13433,6 +13433,45 @@ public void testStringAggExpressionNonConstantSeparator()
);
}
+ @Test
+ public void testStringAnyAggArgValidation()
+ {
+ DruidException e = assertThrows(DruidException.class, () -> testBuilder()
+ .sql("SELECT ANY_VALUE(dim3, 1000, 'true') FROM foo")
+ .queryContext(ImmutableMap.of())
+ .run());
+ assertThat(
+ e,
+ invalidSqlIs(
+ "Cannot apply 'ANY_VALUE' to arguments of type 'ANY_VALUE(<VARCHAR>, <INTEGER>, <CHAR(4)>)'. Supported form(s): 'ANY_VALUE(<expr>, [<maxBytesPerString>, [<aggregateMultipleValues>]])' (line [1], column [8])")
+ );
+ QueryTestBuilder qtb1 = testBuilder()
+ .sql("SELECT ANY_VALUE(dim3, 1000, null) FROM foo")
+ .queryContext(ImmutableMap.of());
+
+ DruidException e1 = assertThrows(DruidException.class, () -> qtb1.run());
Review Comment:
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....
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,22 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ arrayList.add(multiValueSelector.lookupName(rowIndex));
+ });
+ foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
Review Comment:
the other impelementation called `DimensionHandlerUtils.convertObjectToString` even on a single value - is that ok; shouldn't they be in sync?
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1409937844
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java:
##########
@@ -65,6 +67,27 @@ public void aggregate(ByteBuffer buf, int position)
}
}
+ private String readValue(Object object)
+ {
+ if (object == null) {
+ return null;
+ }
+ if (object instanceof List) {
+ List<Object> objectList = (List) object;
+ if (objectList.size() == 0) {
+ return null;
+ }
+ if (objectList.size() == 1) {
+ return DimensionHandlerUtils.convertObjectToString(objectList.get(0));
+ }
+ if (aggregateMultipleValues) {
+ return DimensionHandlerUtils.convertObjectToString(objectList);
+ }
+ return DimensionHandlerUtils.convertObjectToString(objectList.get(0));
Review Comment:
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
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408712280
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,22 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ arrayList.add(multiValueSelector.lookupName(rowIndex));
+ });
+ foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
Review Comment:
made it consistent across all aggregator to return single value in case of array size 1
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1406999287
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java:
##########
@@ -24,38 +24,45 @@
import org.apache.druid.segment.BaseObjectColumnValueSelector;
import org.apache.druid.segment.DimensionHandlerUtils;
+import java.util.List;
+
public class StringAnyAggregator implements Aggregator
{
private final BaseObjectColumnValueSelector valueSelector;
private final int maxStringBytes;
private boolean isFound;
private String foundValue;
+ final boolean aggregateMultipleValues;
Review Comment:
fixed
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408713691
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,22 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ arrayList.add(multiValueSelector.lookupName(rowIndex));
+ });
+ foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
+ }
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
+ }
Review Comment:
good point, moved to method in all aggregators
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1407001551
##########
processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactoryTest.java:
##########
@@ -72,43 +68,107 @@ public void canVectorizeWithoutCapabilitiesShouldReturnTrue()
@Test
public void factorizeVectorWithoutCapabilitiesShouldReturnAggregatorWithMultiDimensionSelector()
{
- Mockito.doReturn(null).when(vectorSelectorFactory).getColumnCapabilities(FIELD_NAME);
- Mockito.doReturn(singleValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeSingleValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
@Test
public void factorizeVectorWithUnknownCapabilitiesShouldReturnAggregatorWithMultiDimensionSelector()
{
- Mockito.doReturn(multiValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeMultiValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
@Test
public void factorizeVectorWithMultipleValuesCapabilitiesShouldReturnAggregatorWithMultiDimensionSelector()
{
- Mockito.doReturn(ColumnCapabilities.Capable.TRUE).when(capabilities).hasMultipleValues();
- Mockito.doReturn(multiValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeMultiValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
@Test
public void factorizeVectorWithoutMultipleValuesCapabilitiesShouldReturnAggregatorWithSingleDimensionSelector()
{
- Mockito.doReturn(ColumnCapabilities.Capable.FALSE).when(capabilities).hasMultipleValues();
- Mockito.doReturn(singleValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeSingleValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
+
+ @Test
+ public void testFactorize()
+ {
+ Aggregator res = target.factorize(new TestColumnSelectorFactory());
+ Assert.assertTrue(res instanceof StringAnyAggregator);
+ res.aggregate();
+ Assert.assertEquals(null, res.get());
+ StringAnyVectorAggregator vectorAggregator = target.factorizeVector(vectorSelectorFactory);
+ Assert.assertTrue(vectorAggregator.aggregateMultipleValues);
+ }
+
+ @Test
+ public void testSvdStringAnyAggregator()
+ {
+ TestColumnSelectorFactory columnSelectorFactory = new TestColumnSelectorFactory();
+ Aggregator res = target.factorize(columnSelectorFactory);
+ Assert.assertTrue(res instanceof StringAnyAggregator);
+ columnSelectorFactory.moveSelectorCursorToNext();
+ res.aggregate();
+ Assert.assertEquals("CCCC", res.get());
+ }
+
+ @Test
+ public void testMvdStringAnyAggregator()
+ {
+ TestColumnSelectorFactory columnSelectorFactory = new TestColumnSelectorFactory();
+ Aggregator res = target.factorize(columnSelectorFactory);
+ Assert.assertTrue(res instanceof StringAnyAggregator);
+ columnSelectorFactory.moveSelectorCursorToNext();
+ columnSelectorFactory.moveSelectorCursorToNext();
+ res.aggregate();
Review Comment:
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.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1405788337
##########
processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java:
##########
@@ -198,4 +189,77 @@
Assert.assertEquals("1.0", result);
}
+
+ @Test
+ public void testMvds()
+ {
+ List<String> mvd = Lists.newArrayList("AAAA", "AAAAB", "AAAC");
+ final Object[] mvds = {null, "CCCC", mvd, "BBBB", "EEEE"};
+ Integer maxStringBytes = 1024;
+
+ TestObjectColumnSelector<Object> objectColumnSelector = new TestObjectColumnSelector<>(mvds);
+
+ StringAnyBufferAggregator agg = new StringAnyBufferAggregator(
+ objectColumnSelector,
+ maxStringBytes, true
+ );
+
+ ByteBuffer buf = ByteBuffer.allocate(factory.getMaxIntermediateSize() * 2);
+ int position = 0;
+
+ int[] positions = new int[]{0, 1, 43, 100, 189};
+ Arrays.stream(positions).forEach(i -> agg.init(buf, i));
+
+ //noinspection ForLoopReplaceableByForEach
+ for (int i = 0; i < mvds.length; i++) {
+ aggregateBuffer(objectColumnSelector, agg, buf, positions[i]);
+ }
+ String result = ((String) agg.get(buf, position));
+ Assert.assertNull(result);
+
+ for (int i = 0; i < positions.length; i++) {
+ if (i == 2) {
+ Assert.assertEquals(mvd.toString(), agg.get(buf, positions[2]));
+ } else {
+ Assert.assertEquals(mvds[i], agg.get(buf, positions[i]));
+ }
+ }
+ }
+
+ @Test
+ public void testMvdsWithCustomAggregate()
+ {
+ List<String> mvd = Lists.newArrayList("AAAA", "AAAAB", "AAAC");
+ final Object[] mvds = {null, "CCCC", mvd, "BBBB", "EEEE"};
+ Integer maxStringBytes = 1024;
Review Comment:
## Boxed variable is never null
The variable 'maxStringBytes' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Integer'.
[Show more details](https://github.com/apache/druid/security/code-scanning/6005)
##########
processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java:
##########
@@ -198,4 +189,77 @@
Assert.assertEquals("1.0", result);
}
+
+ @Test
+ public void testMvds()
+ {
+ List<String> mvd = Lists.newArrayList("AAAA", "AAAAB", "AAAC");
+ final Object[] mvds = {null, "CCCC", mvd, "BBBB", "EEEE"};
+ Integer maxStringBytes = 1024;
Review Comment:
## Boxed variable is never null
The variable 'maxStringBytes' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Integer'.
[Show more details](https://github.com/apache/druid/security/code-scanning/6004)
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408712755
##########
docs/querying/sql-functions.md:
##########
@@ -50,7 +50,7 @@ Calculates the arc cosine of a numeric expression.
## ANY_VALUE
-`ANY_VALUE(expr, [maxBytesPerValue])`
+`ANY_VALUE(expr, [maxBytesPerValue, aggregateMultipleValues])`
Review Comment:
you are right, fixed the docs.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1406899167
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,23 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ String chopped = StringUtils.fastLooseChop(multiValueSelector.lookupName(rowIndex), maxStringBytes);
Review Comment:
i don't think you need to call `fastLooseChop` here because `putValue` uses `StringUtils.toUtf8WithLimit` which achieves the same thing
##########
processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactoryTest.java:
##########
@@ -72,43 +68,107 @@ public void canVectorizeWithoutCapabilitiesShouldReturnTrue()
@Test
public void factorizeVectorWithoutCapabilitiesShouldReturnAggregatorWithMultiDimensionSelector()
{
- Mockito.doReturn(null).when(vectorSelectorFactory).getColumnCapabilities(FIELD_NAME);
- Mockito.doReturn(singleValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeSingleValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
@Test
public void factorizeVectorWithUnknownCapabilitiesShouldReturnAggregatorWithMultiDimensionSelector()
{
- Mockito.doReturn(multiValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeMultiValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
@Test
public void factorizeVectorWithMultipleValuesCapabilitiesShouldReturnAggregatorWithMultiDimensionSelector()
{
- Mockito.doReturn(ColumnCapabilities.Capable.TRUE).when(capabilities).hasMultipleValues();
- Mockito.doReturn(multiValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeMultiValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
@Test
public void factorizeVectorWithoutMultipleValuesCapabilitiesShouldReturnAggregatorWithSingleDimensionSelector()
{
- Mockito.doReturn(ColumnCapabilities.Capable.FALSE).when(capabilities).hasMultipleValues();
- Mockito.doReturn(singleValueDimensionVectorSelector)
- .when(vectorSelectorFactory)
- .makeSingleValueDimensionSelector(any());
StringAnyVectorAggregator aggregator = target.factorizeVector(vectorSelectorFactory);
Assert.assertNotNull(aggregator);
}
+
+ @Test
+ public void testFactorize()
+ {
+ Aggregator res = target.factorize(new TestColumnSelectorFactory());
+ Assert.assertTrue(res instanceof StringAnyAggregator);
+ res.aggregate();
+ Assert.assertEquals(null, res.get());
+ StringAnyVectorAggregator vectorAggregator = target.factorizeVector(vectorSelectorFactory);
+ Assert.assertTrue(vectorAggregator.aggregateMultipleValues);
+ }
+
+ @Test
+ public void testSvdStringAnyAggregator()
+ {
+ TestColumnSelectorFactory columnSelectorFactory = new TestColumnSelectorFactory();
+ Aggregator res = target.factorize(columnSelectorFactory);
+ Assert.assertTrue(res instanceof StringAnyAggregator);
+ columnSelectorFactory.moveSelectorCursorToNext();
+ res.aggregate();
+ Assert.assertEquals("CCCC", res.get());
+ }
+
+ @Test
+ public void testMvdStringAnyAggregator()
+ {
+ TestColumnSelectorFactory columnSelectorFactory = new TestColumnSelectorFactory();
+ Aggregator res = target.factorize(columnSelectorFactory);
+ Assert.assertTrue(res instanceof StringAnyAggregator);
+ columnSelectorFactory.moveSelectorCursorToNext();
+ columnSelectorFactory.moveSelectorCursorToNext();
+ res.aggregate();
Review Comment:
why does it increment cursor twice before aggregating?
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -158,7 +162,8 @@ AggregatorFactory createAggregatorFactory(
case DOUBLE:
return new DoubleAnyAggregatorFactory(name, fieldName);
case STRING:
- return new StringAnyAggregatorFactory(name, fieldName, maxStringBytes);
+ case COMPLEX:
+ return new StringAnyAggregatorFactory(name, fieldName, maxStringBytes, aggregateMultipleValues);
Review Comment:
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
##########
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;
Review Comment:
why protected? (same applies to `maxStringBytes` too i think...)
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java:
##########
@@ -24,38 +24,45 @@
import org.apache.druid.segment.BaseObjectColumnValueSelector;
import org.apache.druid.segment.DimensionHandlerUtils;
+import java.util.List;
+
public class StringAnyAggregator implements Aggregator
{
private final BaseObjectColumnValueSelector valueSelector;
private final int maxStringBytes;
private boolean isFound;
private String foundValue;
+ final boolean aggregateMultipleValues;
Review Comment:
nit: private
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -274,13 +299,31 @@ public Aggregation toDruidAggregation(
args.size()
);
}
-
return Aggregation.create(
Collections.singletonList(theAggFactory),
finalizeAggregations ? new FinalizingFieldAccessPostAggregator(name, aggregatorName) : null
);
}
+ private Integer getMaxStringBytes(
+ final List<RexNode> rexNodes,
+ final AggregateCall aggregateCall,
+ final PlannerContext plannerContext
Review Comment:
this method seems strange to pass `List<RexNode>`, and only uses `rexNodes.get(1)` from it, please pass the `RexNode` to check directly
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1407000710
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,23 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ String chopped = StringUtils.fastLooseChop(multiValueSelector.lookupName(rowIndex), maxStringBytes);
Review Comment:
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.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408712501
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java:
##########
@@ -34,11 +35,13 @@ public class StringAnyBufferAggregator implements BufferAggregator
private final BaseObjectColumnValueSelector valueSelector;
private final int maxStringBytes;
+ final boolean aggregateMultipleValues;
Review Comment:
fixed
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1407001862
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,23 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ String chopped = StringUtils.fastLooseChop(multiValueSelector.lookupName(rowIndex), maxStringBytes);
Review Comment:
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.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on PR #15434:
URL: https://github.com/apache/druid/pull/15434#issuecomment-1828967863
Addressed all comments.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "soumyava (via GitHub)" <gi...@apache.org>.
soumyava commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1407200355
##########
processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java:
##########
@@ -44,17 +51,14 @@ public void testBufferAggregate()
{
final String[] strings = {"AAAA", "BBBB", "CCCC", "DDDD", "EEEE"};
- Integer maxStringBytes = 1024;
+ int maxStringBytes = 1024;
Review Comment:
nit. final
##########
processing/src/test/java/org/apache/druid/query/aggregation/AggregatorFactoryTest.java:
##########
@@ -307,7 +307,7 @@ public void testWithName()
// string aggregators
new StringFirstAggregatorFactory("col", "col", null, 1024),
new StringLastAggregatorFactory("col", "col", null, 1024),
- new StringAnyAggregatorFactory("col", "col", 1024),
+ new StringAnyAggregatorFactory("col", "col", 1024, true),
Review Comment:
Just for sake of completeness can we add a test with false as the 3rd paramter as well ?
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java:
##########
@@ -34,11 +35,13 @@ public class StringAnyBufferAggregator implements BufferAggregator
private final BaseObjectColumnValueSelector valueSelector;
private final int maxStringBytes;
+ final boolean aggregateMultipleValues;
Review Comment:
nit. private
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -43,11 +46,13 @@ public class StringAnyVectorAggregator implements VectorAggregator
@Nullable
private final MultiValueDimensionVectorSelector multiValueSelector;
private final int maxStringBytes;
+ final boolean aggregateMultipleValues;
Review Comment:
nit. private
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408872271
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java:
##########
@@ -24,38 +24,57 @@
import org.apache.druid.segment.BaseObjectColumnValueSelector;
import org.apache.druid.segment.DimensionHandlerUtils;
+import java.util.List;
+
public class StringAnyAggregator implements Aggregator
{
private final BaseObjectColumnValueSelector valueSelector;
private final int maxStringBytes;
private boolean isFound;
private String foundValue;
+ private final boolean aggregateMultipleValues;
- public StringAnyAggregator(BaseObjectColumnValueSelector valueSelector, int maxStringBytes)
+ public StringAnyAggregator(BaseObjectColumnValueSelector valueSelector, int maxStringBytes, boolean aggregateMultipleValues)
{
this.valueSelector = valueSelector;
this.maxStringBytes = maxStringBytes;
this.foundValue = null;
this.isFound = false;
+ this.aggregateMultipleValues = aggregateMultipleValues;
}
@Override
public void aggregate()
{
if (!isFound) {
final Object object = valueSelector.getObject();
- foundValue = DimensionHandlerUtils.convertObjectToString(object);
- if (foundValue != null && foundValue.length() > maxStringBytes) {
- foundValue = foundValue.substring(0, maxStringBytes);
- }
+ foundValue = StringUtils.fastLooseChop(readValue(object), maxStringBytes);
isFound = true;
}
}
+ private String readValue(final Object object)
+ {
+ if (object == null) {
+ return null;
+ }
+ if (object instanceof List) {
+ List<Object> objectList = (List) object;
+ if (objectList.size() == 1) {
Review Comment:
added size 0 check. It can be possible that we ingest empty arrays i guess.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408802079
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java:
##########
@@ -24,38 +24,57 @@
import org.apache.druid.segment.BaseObjectColumnValueSelector;
import org.apache.druid.segment.DimensionHandlerUtils;
+import java.util.List;
+
public class StringAnyAggregator implements Aggregator
{
private final BaseObjectColumnValueSelector valueSelector;
private final int maxStringBytes;
private boolean isFound;
private String foundValue;
+ private final boolean aggregateMultipleValues;
- public StringAnyAggregator(BaseObjectColumnValueSelector valueSelector, int maxStringBytes)
+ public StringAnyAggregator(BaseObjectColumnValueSelector valueSelector, int maxStringBytes, boolean aggregateMultipleValues)
{
this.valueSelector = valueSelector;
this.maxStringBytes = maxStringBytes;
this.foundValue = null;
this.isFound = false;
+ this.aggregateMultipleValues = aggregateMultipleValues;
}
@Override
public void aggregate()
{
if (!isFound) {
final Object object = valueSelector.getObject();
- foundValue = DimensionHandlerUtils.convertObjectToString(object);
- if (foundValue != null && foundValue.length() > maxStringBytes) {
- foundValue = foundValue.substring(0, maxStringBytes);
- }
+ foundValue = StringUtils.fastLooseChop(readValue(object), maxStringBytes);
isFound = true;
}
}
+ private String readValue(final Object object)
+ {
+ if (object == null) {
+ return null;
+ }
+ if (object instanceof List) {
+ List<Object> objectList = (List) object;
+ if (objectList.size() == 1) {
Review Comment:
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);
}
```
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408714401
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -244,17 +249,35 @@ public Aggregation toDruidAggregation(
final AggregatorFactory theAggFactory;
switch (args.size()) {
case 1:
- theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null);
+ theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null, true);
break;
case 2:
- int maxStringBytes;
+ Integer maxStringBytes = getMaxStringBytes(rexNodes.get(1), aggregateCall, plannerContext);
+ if (maxStringBytes == null) {
+ return null;
+ }
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ null,
+ outputType,
+ maxStringBytes.intValue(),
+ true
+ );
+ break;
+ case 3:
+ maxStringBytes = getMaxStringBytes(rexNodes.get(1), aggregateCall, plannerContext);
+ if (maxStringBytes == null) {
+ return null;
+ }
+ boolean aggregateMultipleValues;
try {
- maxStringBytes = RexLiteral.intValue(rexNodes.get(1));
+ aggregateMultipleValues = RexLiteral.booleanValue(Preconditions.checkNotNull(rexNodes.get(2)));
}
- catch (AssertionError ae) {
+ catch (Exception ae) {
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()
);
Review Comment:
removed the code as early validation is added
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408711912
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -13433,6 +13433,45 @@ public void testStringAggExpressionNonConstantSeparator()
);
}
+ @Test
+ public void testStringAnyAggArgValidation()
+ {
+ DruidException e = assertThrows(DruidException.class, () -> testBuilder()
+ .sql("SELECT ANY_VALUE(dim3, 1000, 'true') FROM foo")
+ .queryContext(ImmutableMap.of())
+ .run());
+ assertThat(
+ e,
+ invalidSqlIs(
+ "Cannot apply 'ANY_VALUE' to arguments of type 'ANY_VALUE(<VARCHAR>, <INTEGER>, <CHAR(4)>)'. Supported form(s): 'ANY_VALUE(<expr>, [<maxBytesPerString>, [<aggregateMultipleValues>]])' (line [1], column [8])")
+ );
+ QueryTestBuilder qtb1 = testBuilder()
+ .sql("SELECT ANY_VALUE(dim3, 1000, null) FROM foo")
+ .queryContext(ImmutableMap.of());
+
+ DruidException e1 = assertThrows(DruidException.class, () -> qtb1.run());
Review Comment:
added early validation in operandChecker class and removed the number/boolean checks.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408714128
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,22 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ arrayList.add(multiValueSelector.lookupName(rowIndex));
+ });
+ foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
Review Comment:
DimensionHandlerUtils.convertObjectToString handles null value and here foundValue null is also handled.
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408715545
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -244,37 +248,38 @@
final AggregatorFactory theAggFactory;
switch (args.size()) {
case 1:
- theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null);
+ theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, null, true);
break;
case 2:
- int maxStringBytes;
- try {
- maxStringBytes = RexLiteral.intValue(rexNodes.get(1));
- }
- catch (AssertionError ae) {
- plannerContext.setPlanningError(
- "The second argument '%s' to function '%s' is not a number",
- rexNodes.get(1),
- aggregateCall.getName()
- );
- return null;
- }
+ Integer maxStringBytes = RexLiteral.intValue(rexNodes.get(1)); // added not null check at the function
Review Comment:
## Boxed variable is never null
The variable 'maxStringBytes' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Integer'.
[Show more details](https://github.com/apache/druid/security/code-scanning/6013)
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1407200862
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -78,7 +84,22 @@ public void aggregate(ByteBuffer buf, int position, int startRow, int endRow)
if (startRow < rows.length) {
IndexedInts row = rows[startRow];
@Nullable
- String foundValue = row.size() == 0 ? null : multiValueSelector.lookupName(row.get(0));
+ String foundValue;
+ if (row.size() == 0) {
+ foundValue = null;
+ } else if (aggregateMultipleValues) {
+ if (row.size() > 1) {
+ List<String> arrayList = new ArrayList<>();
+ row.forEach(rowIndex -> {
+ arrayList.add(multiValueSelector.lookupName(rowIndex));
+ });
+ foundValue = DimensionHandlerUtils.convertObjectToString(arrayList);
+ } else {
+ foundValue = multiValueSelector.lookupName(row.get(0));
Review Comment:
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?
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408712379
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyVectorAggregator.java:
##########
@@ -43,11 +46,13 @@ public class StringAnyVectorAggregator implements VectorAggregator
@Nullable
private final MultiValueDimensionVectorSelector multiValueSelector;
private final int maxStringBytes;
+ final boolean aggregateMultipleValues;
Review Comment:
fixed
##########
processing/src/test/java/org/apache/druid/query/aggregation/AggregatorFactoryTest.java:
##########
@@ -307,7 +307,7 @@ public void testWithName()
// string aggregators
new StringFirstAggregatorFactory("col", "col", null, 1024),
new StringLastAggregatorFactory("col", "col", null, 1024),
- new StringAnyAggregatorFactory("col", "col", 1024),
+ new StringAnyAggregatorFactory("col", "col", 1024, true),
Review Comment:
added
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1408713574
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java:
##########
@@ -52,7 +55,13 @@ public void aggregate(ByteBuffer buf, int position)
{
if (buf.getInt(position) == NOT_FOUND_FLAG_VALUE) {
final Object object = valueSelector.getObject();
- String foundValue = DimensionHandlerUtils.convertObjectToString(object);
+ String foundValue;
+ if (object != null && object instanceof List && !aggregateMultipleValues) {
+ List<Object> objectList = (List) object;
+ foundValue = objectList.size() > 0 ? DimensionHandlerUtils.convertObjectToString(objectList.get(0)) : null;
Review Comment:
nope, we check the flag aggregateMultipleValues and return all elements stringified
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "soumyava (via GitHub)" <gi...@apache.org>.
soumyava merged PR #15434:
URL: https://github.com/apache/druid/pull/15434
--
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] Enabling aggregateMultipleValues in all StringAnyAggregators (druid)
Posted by "pranavbhole (via GitHub)" <gi...@apache.org>.
pranavbhole commented on code in PR #15434:
URL: https://github.com/apache/druid/pull/15434#discussion_r1409687634
##########
processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java:
##########
@@ -65,6 +67,27 @@ public void aggregate(ByteBuffer buf, int position)
}
}
+ private String readValue(Object object)
+ {
+ if (object == null) {
+ return null;
+ }
+ if (object instanceof List) {
+ List<Object> objectList = (List) object;
+ if (objectList.size() == 0) {
+ return null;
+ }
+ if (objectList.size() == 1) {
+ return DimensionHandlerUtils.convertObjectToString(objectList.get(0));
+ }
+ if (aggregateMultipleValues) {
+ return DimensionHandlerUtils.convertObjectToString(objectList);
+ }
+ return DimensionHandlerUtils.convertObjectToString(objectList.get(0));
Review Comment:
@clintropolis Thank you Clint. Is it ok to leave this extra safety check in the code ?
--
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