Skip to content

Fix the companion function changes step behavior #13

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

Jump to bottom
Merged
devavret merged 1 commit into from
May 21, 2025
Merged

Fix the companion function changes step behavior #13

devavret merged 1 commit into from
May 21, 2025

Conversation

@jinchengchenghh
Copy link

@jinchengchenghh jinchengchenghh commented May 6, 2025 •
edited
Loading

Before that, the count result is incorrect. For companion aggregate function, the step of all the aggregates can be different, facebookincubator#12830 (comment), in this example, HashAggregate(keys=[], functions=[merge_count(l_quantity#80), partial_count(distinct l_partkey#77L)], output=[count#166L, count#169L]), so we cannot use a single step for HashAggregation

the step is SINGLE the kind is sum_merge_extract
the step is SINGLE the kind is sum_merge_extract
the step is SINGLE the kind is count_merge_extract
add input 14count reduce stepSINGLE
count input 0: {11933.739999999998, -4828.52, 8}
1: {13683.73, -100.96000000000026, 6}
2: {7070.17, 289.63, 8}
3: {13205.7, -952.0400000000004, 8}
4: {7017.68, 4809.41, 5}
5: {129.12, -27.13000000000001, 2}
6: {23060.460000000003, 6369.129999999999, 10}
7: {15549.43, -6224.07, 8}
8: {8111.89, -14071.19, 4}
9: {12510.16, -309.84000000000106, 8}
10: {4638.43, -7112.44, 5}
11: {9982.08, -4268.39, 7}
12: {11483.15, 2080.0799999999995, 8}
13: {1447.4599999999998, -20.260000000000105, 3}
get the output 1
[14,139823.2,-24366.590000000004]                                               
Elapsed time: 4.389513118 seconds

@copy-pr-bot
Copy link

copy-pr-bot bot commented May 6, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@bdice
Copy link

bdice commented May 7, 2025

Would this failure be reproducible in a Velox test? Could we add a test?

@bdice
Copy link

bdice commented May 7, 2025

/ok to test 27e6113

@devavret
Copy link

devavret commented May 7, 2025 •
edited
Loading

@jinchengchenghh Thanks for this PR. Can you add a bit of context on what these functions are? I wasn't aware that functions can dictate their step. This will likely affect streaming aggregations #6 because they rely on the step of the plannode.

@GregoryKimball GregoryKimball mentioned this pull request May 7, 2025
Closed
jinchengchenghh
jinchengchenghh commented May 14, 2025
View reviewed changes
velox/experimental/cudf/exec/CudfHashAggregation.cpp
}
}

static const std::unordered_map<std::string, core::AggregationNode::Step>
Copy link
Author

@jinchengchenghh jinchengchenghh May 14, 2025

Choose a reason for hiding this comment

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

bool isPartialOutput(core::AggregationNode::Step step) {
return step == core::AggregationNode::Step::kPartial ||
step == core::AggregationNode::Step::kIntermediate;
}, when the companion function is count_merge_extract(step Final), it is not partial and cannot flush the final output. Please throw exception for count_merge_extract

jinchengchenghh
jinchengchenghh commented May 19, 2025
View reviewed changes
velox/experimental/cudf/exec/CudfHashAggregation.cpp Outdated
if (partialOutput_ &&
partialOutput_->estimateFlatSize() >
maxPartialAggregationMemoryUsage_) {
if (hasCompanionAggs_ ||
Copy link
Author

@jinchengchenghh jinchengchenghh May 19, 2025

Choose a reason for hiding this comment

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

Then remove the hasCompanionAggs_ here is ok

Copy link

@devavret devavret May 19, 2025

Choose a reason for hiding this comment

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

9733f09

jinchengchenghh
jinchengchenghh commented May 19, 2025
View reviewed changes
velox/experimental/cudf/exec/CudfHashAggregation.cpp
core::AggregationNode::Step step) {
for (const auto& [k, v] : companionStep) {
if (folly::StringPiece(kind).endsWith(k)) {
step = v;
Copy link
Author

@jinchengchenghh jinchengchenghh May 19, 2025

Choose a reason for hiding this comment

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

break;

@jinchengchenghh @devavret
Fix CudfHashAggregation operator to work with companion aggs
0c06254 
fix

Changes to make companion aggs work post streaming aggs changes

working test

Make the node behave like a final node if there's any _merge_extract aggs.

remove hasCompanionAggs
@jinchengchenghh
Copy link
Author

jinchengchenghh commented May 21, 2025

I verified locally, this PR can pass the TPCDS Q95, we could merge it now. Thanks for your fix. @devavret

@devavret devavret merged commit 773208d into rapidsai:velox-cudf May 21, 2025
VinithKrishnan pushed a commit to VinithKrishnan/velox-rapidsai that referenced this pull request Jun 29, 2025
@facebook-github-bot
fix(vector): VectorMaker-inl.h: fix llvm-19-exposed UBSAN signed-inte…
5108202 
…ger-overflow (facebookincubator#13831)

Summary:
Pull Request resolved: facebookincubator#13831

This avoids the following errors:

```
fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41: runtime error: negation of -9223372036854775808 cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself
    #0 0x000000346ce5 in std::abs(long) fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56
    rapidsai#1 0x000000345879 in std::shared_ptr<facebook::velox::BiasVector<facebook::velox::test::EvalTypeHelper<long>::Type>> facebook::velox::test::VectorMaker::biasVector<long>(std::vector<std::optional<long>, std::allocator<std::optional<long>>> const&) fbcode/velox/vector/tests/utils/VectorMaker-inl.h:58
    rapidsai#2 0x000000344d34 in facebook::velox::test::BiasVectorErrorTest::errorTest(std::vector<std::optional<long>, std::allocator<std::optional<long>>>) fbcode/velox/vector/tests/BiasVectorTest.cpp:39
    rapidsai#3 0x00000033ec99 in facebook::velox::test::BiasVectorErrorTest_checkRangeTooLargeError_Test::TestBody() fbcode/velox/vector/tests/BiasVectorTest.cpp:44
    rapidsai#4 0x7fe0a2342c46 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) fbsource/src/gtest.cc:2727
    rapidsai#5 0x7fe0a234275d in testing::Test::Run() fbsource/src/gtest.cc:2744
    rapidsai#6 0x7fe0a2345fb3 in testing::TestInfo::Run() fbsource/src/gtest.cc:2890
    rapidsai#7 0x7fe0a234c8eb in testing::TestSuite::Run() fbsource/src/gtest.cc:3068
    rapidsai#8 0x7fe0a237b52b in testing::internal::UnitTestImpl::RunAllTests() fbsource/src/gtest.cc:6059
    rapidsai#9 0x7fe0a237a0a2 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) fbsource/src/gtest.cc:2727
    rapidsai#10 0x7fe0a23797f5 in testing::UnitTest::Run() fbsource/src/gtest.cc:5599
    rapidsai#11 0x7fe0a2239800 in RUN_ALL_TESTS() fbsource/gtest/gtest.h:2334
    rapidsai#12 0x7fe0a223952c in main fbcode/common/gtest/LightMain.cpp:20
    rapidsai#13 0x7fe09ec2c656 in __libc_start_call_main /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    rapidsai#14 0x7fe09ec2c717 in __libc_start_main@GLIBC_2.2.5 /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../csu/libc-start.c:409:3
    rapidsai#15 0x00000033d8b0 in _start /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/x86_64/start.S:116

UndefinedBehaviorSanitizer: signed-integer-overflow fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41
```
Avoid overflow by using the expression (static_cast<uint64_t>(1) + ~static_cast<uint64_t>(min)) to calculate the absolute value of min without using std::abs

Reviewed By: dmm-fb, peterenescu

Differential Revision: D76901449

fbshipit-source-id: 7eb3bd0f83e42f44cdf34ea1759f3aa9e1042dae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdice bdice Awaiting requested review from bdice bdice is a code owner

@karthikeyann karthikeyann Awaiting requested review from karthikeyann karthikeyann is a code owner

@devavret devavret Awaiting requested review from devavret devavret is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jinchengchenghh @bdice @devavret