Vector Filtered Aggregates return the default buffer init values when no rows match the filter predicate

[damnMeddlingKid]

Affected Version

Tested on 0.20.0

Description

Not sure if this is a bug
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.

[abhishekagarwal87]

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.

[damnMeddlingKid]

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.

[ericxiao251]

Hey, can I take this one :)?

[ericxiao251]

@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: #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

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

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.

[benkrug]

Why 0? If there are no matching rows, shouldn't the result be that no rows are returned, and not a value?

[ericxiao251]

@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:

---

This is from the documentation:

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).

[damnMeddlingKid]

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.

[clintropolis]

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?

[ericxiao251]

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?

[gianm]

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).

[ericxiao251]

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 :).

[github-actions]

This issue has been marked as stale due to 280 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If this issue is still
relevant, please simply write any comment. Even if closed, you can still revive the
issue at any time or discuss it on the dev@druid.apache.org list.
Thank you for your contributions.