You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/12/05 05:05:52 UTC
[GitHub] [druid] damnMeddlingKid opened a new issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
damnMeddlingKid opened a new issue #10644:
URL: https://github.com/apache/druid/issues/10644
### Affected Version
Tested on 0.20.0
### Description
When executing a filtered aggregate that doesn't match any rows druid will return the init value of the aggregate involved. For example for the LongMaxVectorAggregator druid will return Long.MIN as the result of the aggregation because this is what the aggregator was initialized to https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/query/aggregation/LongMaxVectorAggregator.java#L39
Steps to reproduce
1) set druid.generic.useDefaultValueForNull=true
2) Run a query like
```
SELECT MAX(__time) as t1, MAX(__time) FILTER(WHERE dim1 = 'non_existing') as t2 FROM druid.foo
```
this is a filtered aggregate on a time column where the filter doesn't match anything. I'm not sure what the expected result should be here, but i was surprised to find that the actual result is
```
"t1":"2001-01-03T00:00:00.000Z", "t2":"-292275055-05-16T16:47:04.192Z"
```
It sounds like according to the documentation LongMax should return 0 as a default value when theres nothing to aggregate.
----------------------------------------------------------------
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.
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
[GitHub] [druid] damnMeddlingKid commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
damnMeddlingKid commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-840043076
> Why 0? If there are no matching rows, shouldn't the result be that no rows are returned, and not a value?
We should consider all codepaths, You could be running this as a filtered aggregate along with other columns that do have values.
It looks like in the case of default value replacement the aggregates are initialized with sentinel values that help the user distinguish "no result" because the plan can't return "null" in this case.
I think the behaviour should be documented so that its doesn't come as a surprise to users.
--
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.
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
[GitHub] [druid] abhishekagarwal87 commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-739488916
Interesting! `NumericAnyVectorAggregator` does it correctly by using a bit to differentiate initialized value vs a value from any row. May be, LongMaxVectorAggregator needs to do the same.
By the way, what is the output of the query if you set `druid.generic.useDefaultValueForNull` to `false`? It should be `null` ideally.
----------------------------------------------------------------
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.
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
[GitHub] [druid] ericxiao251 commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
ericxiao251 commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-839956260
@abhishekagarwal87 do you know who the best person to reach out to about this issue is? Copying and pasting my email to the dev mailing list... I am not sure if this is actually a bug in the code or undocumented behaviour:
---
Hi Developers,
I am looking for some guidance on this situation described here: https://github.com/apache/druid/issues/10644. I am trying to understand if the behavior is expected and the documentation for the runtime parameter `druid.generic.useDefaultValueForNull` is not updated to reflect the behavior OR this is an actual bug in the engine.
As described in the issue, there are a couple of scenarios that don’t seem right according to the documentation:
Example when results are `Long.MIN_VALUE`
1) perform a MAX aggregation function
2) on empty rows
3) `druid.generic.useDefaultValueForNull=true`
```sql
Query: SELECT MAX(l1) FILTER(WHERE dim1 = 'non_existing') FROM druid.foo
Result: -9223372036854775808
```
Reference to explicitly set value in source code:
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/query/aggregation/LongMaxAggregatorFactory.java#L56-L60
Example when results are 0
1) perform a SUM aggregation function
2) on empty rows
3) `druid.generic.useDefaultValueForNull=true`
```sql
Query: SELECT SUM(l1) FILTER(WHERE dim1 = 'non_existing') FROM druid.foo
Result: 0
```
Reference to explicitly set value in source code:
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java#L56-L60
From the Druid documentation link:
Property: `druid.generic.useDefaultValueForNull`
Description: When set to true, null values will be stored as `‘’` for string columns and 0 for numeric columns. Set to false to store and query data in SQL compatible mode.
Default: true
The documentation does not explicitly mention what the expected results of an aggregation query would be but I expected the results from both of the above queries to be 0; not `Long.MIN_VALUE` in some instances.
Should the documentation be updated or should the code be fixed? I can put in either fix, but want to know which the community thinks is appropriate.
--
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.
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
[GitHub] [druid] ericxiao251 commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
ericxiao251 commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-840038909
@benkrug I don't have much context as to why the results are what they are but I think that is the intentions of the runtime parameter `druid.generic.useDefaultValueForNull`; return a non-null value (when set to True) or `NULL` (when set to False).
Whether the non-null value should be 0 or `Long.MIN_VALUE` is what I am trying to figure out. In Presto SQL... this is what I get:
<img width="324" alt="Screen Shot 2021-05-12 at 3 23 20 PM" src="https://user-images.githubusercontent.com/9758510/118032727-fd586780-b335-11eb-9940-bf2aeec73d16.png">
---
This is from the [documentation](https://druid.apache.org/docs/latest/configuration/index.html#sql-compatible-null-handling):
```
When set to true, null values will be stored as '' for string columns and 0 for numeric columns. Set to false to store and query data in SQL compatible mode.
```
It worries that the default value gives the non SQL expected value (Null).
--
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.
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
[GitHub] [druid] damnMeddlingKid commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
damnMeddlingKid commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-740055257
> By the way, what is the output of the query if you set `druid.generic.useDefaultValueForNull` to `false`? It should be `null` ideally.
yup, in this case the planner uses the `NullableNumericVectorAggregator` which initializes the buffer with a null byte and returns null if no aggregation occurs.
----------------------------------------------------------------
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.
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
[GitHub] [druid] benkrug commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
benkrug commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-840031182
Why 0? If there are no matching rows, shouldn't the result be that no rows are returned, and not a 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.
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
[GitHub] [druid] ericxiao251 commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
ericxiao251 commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-845958767
Awesome, I'll put up a PR to reference the new documentations soon.
> We aren't doing anything special to handle or detect integer overflow in sums, so it would overflow in the usual way (things would wrap around to negative, since integer math in Java is signed).
Got it, I'm still new to the project :).
--
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.
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
[GitHub] [druid] damnMeddlingKid edited a comment on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
damnMeddlingKid edited a comment on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-840043076
> Why 0? If there are no matching rows, shouldn't the result be that no rows are returned, and not a value?
We should consider all codepaths, You could be running this as a filtered aggregate along with other columns that do have values.
It looks like in the case of default value replacement the aggregates are initialized with sentinel values that help the user distinguish "no result" because the plan can't return "null" in this case.
I think the behaviour should be documented so that it doesn't come as a surprise to users.
--
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.
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
[GitHub] [druid] ericxiao251 commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
ericxiao251 commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-840644078
> Or is this just confusion since none of this was previously explicitly documented so the only reference was the documentation on the two null handling modes?
@clintropolis ah I missed the new documentation you created! I think that is exactly it, the documentation on the druid homepage (not Github) doesn't explain clearly the expected behaviour for the various aggregate functions and their defaults. If we could link your new documentation somewhere in the druid website in this section that would be great: https://druid.apache.org/docs/latest/configuration/index.html#sql-compatible-null-handling. I can put a PR up for that :).
---
I don't think I fully understand the downstream complications of an aggregation returning a null value, so I also don't have strong opinions about what the value should be when we see a null value.
When an aggregate result overflows, what are the results? Would it be `LONG.MIN_VALUE`/`LONG.MAX_VALUE` for example? Would this be the only confusing scenario if we used the two min and max values?
--
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.
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
[GitHub] [druid] ericxiao251 commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
ericxiao251 commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-830859076
Hey, can I take this one :)?
--
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.
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
[GitHub] [druid] gianm commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
gianm commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-845165898
> If we could link your new documentation somewhere in the druid website in this section that would be great: https://druid.apache.org/docs/latest/configuration/index.html#sql-compatible-null-handling. I can put a PR up for that :).
That sounds like a good idea.
> When an aggregate result overflows, what are the results? Would it be LONG.MIN_VALUE/LONG.MAX_VALUE for example? Would this be the only confusing scenario if we used the two min and max values?
Do you mean, like, if a SUM or COUNT overflows? We aren't doing anything special to handle or detect integer overflow in sums, so it would overflow in the usual way (things would wrap around to negative, since integer math in Java is signed).
--
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.
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
[GitHub] [druid] clintropolis commented on issue #10644: Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate
Posted by GitBox <gi...@apache.org>.
clintropolis commented on issue #10644:
URL: https://github.com/apache/druid/issues/10644#issuecomment-840116328
Hi @damnMeddlingKid , @ericxiao251, (and sorry I missed the mailing list thread)
I did document this behavior recently in #11188, which includes a new column that lists out all of the initial aggregator values in both modes, https://github.com/apache/druid/blob/master/docs/querying/sql.md#aggregation-functions.
Whether or not this is the most correct thing for min/max to be doing I would consider this fair game to be up for debate. As mentioned in this thread already, in SQL compatible null handling mode these aggregators are initialized to the null value, and so not aggregating any rows will produce the expected `null` result, which in my personal view seems like the only correct thing to do in the case where filters do not match, but that doesn't work for default mode of course.
As alluded to by @abhishekagarwal87, I don't think we could make the 'default' mode min/max aggregators return `0` either without storing some additional information to distinguish not aggregating values (so probably no longer using the primitive numeric base aggregators, or perhaps always using the nullable version and just coercing to 0 later?), or by dropping the very end values and assuming Long.MIN_VALUE and Long.MAX_VALUE don't legitimately exist and translating them to 0 in a finalizer, which would allow them to keep using the numeric primitive aggregator base classes.
"default" null handling mode is often unintuitive I think, especially in SQL queries, and for the most part SQL compatible null handling mode should have behavior that is more consistent with other databases and with what you would expect in SQL. I would much prefer long term to deprecate and eventually remove default value mode, so that this case of no matches always returns null, but it seems reasonable to discuss adjusting the default values for default value mode until then.
I don't have a strong opinion on the default value for min/max... what is the motivation to have 0 instead of an unlikely value returned when nothing matches? Or is this just confusion since none of this was previously explicitly documented so the only reference was the documentation on the two null handling modes?
--
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.
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