Skip to content
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.

Sign up for GitHub

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 return type for sum(REAL) Spark aggregate #9818

Closed
wants to merge 2 commits into from
Closed

Fix return type for sum(REAL) Spark aggregate #9818

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
173f521
spark sql sum(real) return type is double
JkSelf May 15, 2024
ea6a159
Sumfloat overflow
JkSelf May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion velox/functions/sparksql/aggregates/SumAggregate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ exec::AggregateRegistrationResult registerSum(
bool overwrite) {
std::vector<std::shared_ptr<exec::AggregateFunctionSignature>> signatures{
exec::AggregateFunctionSignatureBuilder()
.returnType("real")
.returnType("double")
.intermediateType("double")
.argumentType("real")
.build(),
Expand Down
11 changes: 11 additions & 0 deletions velox/functions/sparksql/aggregates/tests/SumAggregationTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,5 +449,16 @@ TEST_F(SumAggregationTest, decimalRangeOverflow) {
{expected},
{});
}

TEST_F(SumAggregationTest, sumFloat) {
auto data = makeRowVector({makeFlatVector<float>({2.00, 1.00})});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this test fail before the change?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FelixYBW I wonder what was the failure. I tried running this test on 'main' and it passed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:( @JkSelf Did you do the test? Looks the UT can't detect the type mismatch.

The PR does solved the customer issue but the issue is caused by window function validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbasmanova @FelixYBW

When running the sum(float) aggregate window function with gluten, the following error occurs:

Error Source: USER
Error Code: INVALID_ARGUMENT
Reason: Unexpected return type for window function sum(REAL). Expected REAL. Got DOUBLE.
Retriable: False

This is due to a mismatch between the function signatures in Spark and Velox. In Spark, the sum(real) function is expected to return a double type, whereas in Velox, the same function is registered to return a real type, leading to incompatibility. It is hard to reproduce this exception in Velox. I have modified the unit test to trigger an overflow error if the current patch is not applied. Please help to review again. Thanks.

createDuckDbTable({data});

testAggregations(
[&](auto& builder) { builder.values({data}); },
{},
{"spark_sum(c0)"},
"SELECT sum(c0) FROM tmp");
}
} // namespace
} // namespace facebook::velox::functions::aggregate::sparksql::test
Loading