[GLUTEN-4652][VL] Fix min_by/max_by result mismatch #5544
Conversation
yma11
force-pushed
the
max-by
branch
6 times, most recently
from
6065078 to
789591b
Compare
|
@rui-mo Can you help review this PR? Thanks. |
| case _: Average | _: Sum if aggFunc.dataType.isInstanceOf[DecimalType] => | ||
| "row_constructor" | ||
| case _: MaxMinBy => | ||
| "row_constructor_with_all_null" |
There was a problem hiding this comment.
@yma11 Is this change due to the semantics difference between Velox and Spark? I wonder if it is possible to adjust the semantics of the implementation in Velox and avoid introducing more hack in Gluten.
There was a problem hiding this comment.
I think by default, the upstream row_constructor doesn't set null value of the struct, it just take the input children and wrap them as a row. These issues are mainly caused by the additional projects we added, so it's reasonable to handle it at our side.
There was a problem hiding this comment.
I see your point. Could we add a comment to explain why row_constructor_with_all_null is needed by MaxMinBy?
| case _: Average | _: Sum if aggFunc.dataType.isInstanceOf[DecimalType] => | ||
| "row_constructor" | ||
| case _: MaxMinBy => | ||
| "row_constructor_with_all_null" |
There was a problem hiding this comment.
I see your point. Could we add a comment to explain why row_constructor_with_all_null is needed by MaxMinBy?
| #include "velox/expression/SpecialForm.h" | ||
|
|
||
| namespace gluten { | ||
| class RowConstructorWithAllNullCallToSpecialForm : public facebook::velox::exec::FunctionCallToSpecialForm { |
There was a problem hiding this comment.
Is it possible to extend RowConstructorWithNullCallToSpecialForm and reuse most of the code?
yma11
force-pushed
the
max-by
branch
2 times, most recently
from
d522a35 to
bd5ebc7
Compare
|
@rui-mo your comments are addressed. Please help review again. Thanks. |
zhouyuan
changed the title
|
I found the result still not right after applying this patch. Expected result is Actual result is |
|
Also cc @ulysses-you |
What changes were proposed in this pull request?
Fix
min_by/max_byresult mismatch. Takemax_byfor example, we need to keep intermediate result row like<null, 11>which will be compared with another result like<5, 8>and assure final result is<null, 11>.How was this patch tested?
New UT added