Move ValueList to functions/lib/aggregates for Spark reuse

[liujiayi771]

The array_agg function in Spark and Presto has some inconsistent behavior
when handling null values. We need to re-implement Spark's array_agg in Velox.
To be able to reuse ValueList, it needs to be moved to the
functions/lib/aggregates directory first.

The copyValueListToArrayWriter method has also been moved to the
ValueList, making it reusable.

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	123c4cf
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66018d8a81a2fc0008774ddd

[liujiayi771]

Hi @mbasmanova. Could you help to review?

[mbasmanova]

The array_agg function in Spark and Presto has some inconsistent behavior when handling null values. We need to re-implement Spark's array_agg in Velox.

What is the difference in behavior between Presto and Spark?

[liujiayi771]

What is the difference in behavior between Presto and Spark?

The semantics are generally consistent, but there are inconsistencies between Presto and Spark in the handling of null values. Spark always ignores null values in the input, whereas Presto preserves them. Moreover, Presto returns null when all inputs are null, while Spark returns an empty list or set.
I plan to rewrite Spark's array_agg based on the simple aggregate function interface.

[liujiayi771]

Hi @mbasmanova, there is a draft PR #9231 for Spark's array_agg, you could get more details from it. I plan to split the move of ValueList and the implementation of Spark's array_agg into two PRs, although they could be combined into a single PR.

[mbasmanova]

@liujiayi771 Thank you for clarifying. Presto's implementation depends on presto.array_agg.ignore_nulls config flag. Does Spark implementation match Presto's when presto.array_agg.ignore_nulls = true?

[liujiayi771]

Does Spark implementation match Presto's when presto.array_agg.ignore_nulls = true?

There is still some difference. When all inputs are null, the output of Spark is an empty array, while Presto returns null. So I need to use the Non-Default-Null Behavior simple function interface.

[mbasmanova]

@liujiayi771

When all inputs are null, the output of Spark is an empty array, while Presto returns null.

Got it. Thank you for clarifying.

CI is red. Please, take a look.

[liujiayi771]

@mbasmanova I will try to rebase the latest main branch to see if the CI is still red.

[liujiayi771]

@mbasmanova I found that your PR also has these two failing tests. Perhaps these two tests themselves are problematic.

[  FAILED  ] MinMaxByNTest.sortedGlobal
[  FAILED  ] MinMaxByNTest.sortedGroupBy

[mbasmanova]

@liujiayi771 It might be #9200

[mbasmanova]

@kagamiori Wei, would you help review this PR?

[pedroerp]

Looks good to me. Thanks @liujiayi771

[facebook-github-bot]

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

[liujiayi771]

@Yuhta @mbasmanova Did you encounter any issues with the merge? Do I need to rebase with the latest code?

[pedroerp]

@Yuhta @mbasmanova Did you encounter any issues with the merge? Do I need to rebase with the latest code?

There were a few smaller internal fixes required since the header got moved, but I'm merging it today.

[facebook-github-bot]

@pedroerp merged this pull request in 397bc05.

[conbench-facebook]

Conbench analyzed the 1 benchmark run on commit 397bc050.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.