Fail if inputs contain nested nulls in min/max/min_by/max_by Presto aggregates

[duanmeng]

In terms of min/max, processing a single stream of values [1, 2], [2, 3], [3, null], [3, 4] goes like this:

• acc = [1, 2]
• compare(acc, [2, 3]) -> keep acc = [1, 2]
• compare (acc, [3, null]) -> detect that 1 < 3 and do not continue comparing 2 with null - > keep acc = [1, 2]

But processing 2 streams: ([1, 2], [2, 3]) and ([3, null], [3, 4]) goes differently. When processing 2nd stream:

• acc = [3, null]
• compare (acc, [3, 4]) triggers comparison null v. 4 and fails.

For example,

presto> select min(col0) from (values (array [1, 2]), (array [2, 3]), (array [3, null]), (array [3, 4])) as tbl(col0);
 _col0
--------
 [1, 2]
(1 row)

But it will fail if I break them into two aggregates,

presto> select min(col0) from (values (array [1, 2]), (array [2, 3])) as tbl(col0);
 _col0
--------
 [1, 2]
(1 row)

presto> select min(col0) from (values (array [3, null]), (array [3, 4])) as tbl(col0);
Query 20230919_044628_00012_6yp5g failed: ARRAY comparison not supported for arrays with null elements

This behavior is problematic because when a query runs on a cluster it can go either way and therefore, the same query may either succeed or fail. Failures are intermittent and non-deterministic.

We need to make sure that the query always fails to make the query always fail, to make it more semantically clear. This PR adds a check that all inputs to min/max (and other similar functions) do not contain nested nulls before serializing them to the accumulator.

Part of #6314

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	0bbde39
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6513b71114ad8a0008cd34c6

[mbasmanova]

@duanmeng Thank you for working on this fix. Some comments below. Also, would it be possible to add a test that triggers the new check?

[mbasmanova]

@duanmeng Let's also update documentation for min/max to explain that this function doesn't allow input values of complex types that contain nulls.

[duanmeng]

@duanmeng Let's also update documentation for min/max to explain that this function doesn't allow input values of complex types that contain nulls.

@mbasmanova Sure done.

[duanmeng]

@duanmeng Thank you for working on this fix. Some comments below. Also, would it be possible to add a test that triggers the new check?

@mbasmanova I've adjusted the complex-type tests to the pattern that would not throw an exception without this patch.
For example,

auto data = makeRowVector({
      makeNullableMapVector<int64_t, int64_t>({
          {{{1, 1}, {2, 2}}},
          {{{2, std::nullopt}, {2, 3}}},
          {{{4, 50}}},
      }),
  });

VELOX_ASSERT_THROW(
      testAggregations({data}, {}, {"min(c0)", "max(c0)"}, {expected}),
      "MAP comparison not supported for values that contain nulls");

[mbasmanova]

Some comments. CC: @kagamiori

[mbasmanova]

I've adjusted the complex-type tests to the pattern that would not throw an exception without this patch.

Somehow I don't see these changes. Would you double check?

BTW, in a follow-up we are going to remove support for MAP inputs altogether. Hence, it might better to use ARRAY or ROW for these tests.

[duanmeng]

@mbasmanova Hi Masha, I've resolved all the comments according to your guidance.

• Combine check null and check nested nulls into one function
• Use isRawInput and throwOnNestedNulls
• Add two more check null test cases following the principle that would not throw an exception without this patch.

[mbasmanova]

Looks great % a couple minor comments.

[mbasmanova]

Thanks.

[facebook-github-bot]

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@mbasmanova merged this pull request in 05386f6.

[conbench-facebook]

Conbench analyzed the 1 benchmark run on commit 05386f6a.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[mbasmanova]

@duanmeng It looks like there is one more place that needs updating. NonNumericMinMaxAggregateBase::toIntermediate also needs to check complex type inputs and throw if there are nested nulls. Any chance you could help make this change?

[duanmeng]

@duanmeng It looks like there is one more place that needs updating. NonNumericMinMaxAggregateBase::toIntermediate also needs to check complex type inputs and throw if there are nested nulls. Any chance you could help make this change?

@mbasmanova Got it, I will fix this. I should read the doc more carefully. https://facebookincubator.github.io/velox/develop/aggregate-functions.html#:~:text=By%20default%2C%20to,the%20input%20unmodified.

[duanmeng]

@mbasmanova Hi Masha, toIntermediate is a kind of adaptive optimization by abandons partial aggregation that has no meaningful cardinality reduction. For min/max aggregates toIntermediate act as follows,

• Raw data in raw data out (sometimes with dictionary encoding)
• It happens in step ofStep::kPartial
• Its result is the input of another step Step::kIntermediate\Step::kFinal, which would not check nested nulls of the input, causing no null checks for the rest of the raw input.

Do I understand correctly?

[mbasmanova]

@duanmeng Thanks. You got it perfectly. This is exactly what happens.

[mbasmanova]

@duanmeng Meng, do you know if we have create an issue about this in PrestoDB repo?

[duanmeng]

@duanmeng Meng, do you know if we have create an issue about this in PrestoDB repo?

@mbasmanova Hi Masha, I think we do not. I just scanned issue #6314 and found no PrestoDB issue linked to it.