-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix window aggregate result for empty frames #6872
Conversation
✅ Deploy Preview for meta-velox canceled.
|
velox/functions/prestosql/window/tests/SimpleAggregatesTest.cpp
Outdated
Show resolved
Hide resolved
e5db75d
to
cd013d5
Compare
velox/functions/prestosql/window/tests/SimpleAggregatesTest.cpp
Outdated
Show resolved
Hide resolved
020a26e
to
15c728e
Compare
70d3458
to
7c6225a
Compare
7c6225a
to
0a06d2c
Compare
velox/functions/prestosql/window/tests/SimpleAggregatesTest.cpp
Outdated
Show resolved
Hide resolved
418329f
to
b48d06f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pramodsatya
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
velox/functions/prestosql/window/tests/SimpleAggregatesTest.cpp
Outdated
Show resolved
Hide resolved
b48d06f
to
99b9e78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @mbasmanova, addressed the comments.
velox/functions/prestosql/window/tests/SimpleAggregatesTest.cpp
Outdated
Show resolved
Hide resolved
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@pramodsatya Got a lint warning: |
99b9e78
to
dccc69b
Compare
Fixed, thanks for sharing @mbasmanova |
velox/functions/prestosql/window/tests/SimpleAggregatesTest.cpp
Outdated
Show resolved
Hide resolved
dccc69b
to
8879104
Compare
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
@mbasmanova merged this pull request in 560b027. |
Presto returns the default value of the aggregate for rows with empty frames. Velox like DuckDB always returned nulls. Fix Velox behavior to be consistent with Presto.
The default value of aggregates is usually null, but there are aggregates like count which return 0.
Resolves #6416