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

Jump to bottom

Disable aggregate pushdown for decimal type #11298

Closed
wants to merge 7 commits into from
Closed

Disable aggregate pushdown for decimal type #11298

wants to merge 7 commits into from

Conversation

NEUpanning
Copy link
Contributor

@NEUpanning NEUpanning commented Oct 18, 2024 •
edited
Loading

Currently, int64_t enables push-down for decimal type. This PR disables aggregate pushdown for decimal type regardless of c++ type.

Fixes #11290

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 18, 2024
@netlify Netlify
Copy link

netlify bot commented Oct 18, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox ready!

Name Link
🔨 Latest commit fe76952
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6734122950bd9b0008184d30
😎 Deploy Preview https://deploy-preview-11298--meta-velox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@NEUpanning
Copy link
Contributor Author

@Yuhta Could you please help to review this PR? Thanks.

@Yuhta Yuhta changed the title Disable push-down for decimal type Disable aggregate pushdown for decimal type Oct 21, 2024
Yuhta
Yuhta reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@Yuhta Yuhta left a comment

Choose a reason for hiding this comment

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

Can you add a unit test to make sure short decimal aggregation is not push down?

velox/functions/lib/aggregates/SimpleNumericAggregate.h Outdated Show resolved Hide resolved
velox/functions/lib/aggregates/MinMaxAggregateBase.cpp Show resolved Hide resolved
velox/functions/lib/aggregates/MinMaxAggregateBase.cpp Show resolved Hide resolved
velox/functions/lib/aggregates/SimpleNumericAggregate.h Outdated Show resolved Hide resolved
@NEUpanning
Copy link
Contributor Author

NEUpanning commented Oct 22, 2024 •
edited
Loading

@Yuhta I've updated code based on comments and added a unit test. Could you please take a look?

@NEUpanning NEUpanning requested a review from Yuhta October 25, 2024 03:01
@NEUpanning
Copy link
Contributor Author

@Yuhta Just a gentle ping.

@Yuhta
Copy link
Contributor

Yuhta commented Nov 5, 2024

@NEUpanning The test failure seems related: MinMaxTest.maxShortDecimal

@NEUpanning
Copy link
Contributor Author

NEUpanning commented Nov 6, 2024 •
edited
Loading

mayPushdown && args[0]->isLazy()) && !args[0]->type()->isDecimal()

@Yuhta Thanks for pointing out the failing unit test. For MinAggregate and MaxAggregate we should set mayPushdown = false if type is decimal. Otherwise, the unloaded lazyvector could cause test failures when pushdown is disabled by decimal type in SimpleNumericAggregate::updateGroups. I've updated code to resovle it.

@NEUpanning
Copy link
Contributor Author

@Yuhta Could you please help to review this PR again? Thanks.

@Yuhta Yuhta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 12, 2024
@facebook-github-bot
Copy link
Contributor

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

@kevinwilfong
Copy link
Contributor

@NEUpanning It looks like the commit was published a while ago, do you mind rebasing and updating the PR?

@NEUpanning
Copy link
Contributor Author

@kevinwilfong Sure, I've rebased it onto the main branch.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@kevinwilfong merged this pull request in 06ab001.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 06ab0013.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@NEUpanning NEUpanning deleted the fix_decimal_pushdown branch November 14, 2024 02:10
@weiting-chen weiting-chen mentioned this pull request Nov 20, 2024
Open
37 tasks
weiting-chen pushed a commit to weiting-chen/velox that referenced this pull request Nov 23, 2024
@NEUpanning @weiting-chen
Disable aggregate pushdown for decimal type (facebookincubator#11298)
dd25d23 
Summary:
Currently, `int64_t` enables push-down for decimal type. This PR disables aggregate pushdown for decimal type regardless of c++ type.

Fixes facebookincubator#11290

Pull Request resolved: facebookincubator#11298

Reviewed By: Yuhta

Differential Revision: D65834210

Pulled By: kevinwilfong

fbshipit-source-id: 422f7eda8f4184c6fa83055e7cf430ff5053d387
@weiting-chen weiting-chen mentioned this pull request Nov 23, 2024
Merged
weiting-chen added a commit to oap-project/velox that referenced this pull request Nov 23, 2024
@weiting-chen @Yuhta @NEUpanning
[Branch-1.2] Port facebookincubator#10583 facebookincubator#10755 fac…
74857c2 
…ebookincubator#11298 (#505)

* Fix NaN handling for min/max aggreates pushed down to scan (facebookincubator#10583)

Summary:
Pull Request resolved: facebookincubator#10583

This fixes min/max aggregates to handle NaN values correctly when they are pushed down to the scan operator. Specifically, the change ensures that NaN values are considered greater than infinity.

Reviewed By: zacw7

Differential Revision: D60297934

fbshipit-source-id: 3398ba24d6fc70bbd0d8583d2949391b0824099c

* Schema evolution support for reader value hooks (facebookincubator#10755)

Summary:
X-link: facebookincubator/nimble#72

Pull Request resolved: facebookincubator#10755

Currently reader value hook is not considering schema evolution at all, this change fix that.

Reviewed By: kevinwilfong

Differential Revision: D61229494

fbshipit-source-id: 729bb90611fb3164282b524376eda20985a30194

* Disable aggregate pushdown for decimal type (facebookincubator#11298)

Summary:
Currently, `int64_t` enables push-down for decimal type. This PR disables aggregate pushdown for decimal type regardless of c++ type.

Fixes facebookincubator#11290

Pull Request resolved: facebookincubator#11298

Reviewed By: Yuhta

Differential Revision: D65834210

Pulled By: kevinwilfong

fbshipit-source-id: 422f7eda8f4184c6fa83055e7cf430ff5053d387

---------

Co-authored-by: Bikramjeet Vig <bikramjeet@meta.com>
Co-authored-by: Jimmy Lu <jimmylu@meta.com>
Co-authored-by: NEUpanning <emmning@163.com>
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
@NEUpanning @athmaja-n
Disable aggregate pushdown for decimal type (facebookincubator#11298)
b6219af 
Summary:
Currently, `int64_t` enables push-down for decimal type. This PR disables aggregate pushdown for decimal type regardless of c++ type.

Fixes facebookincubator#11290

Pull Request resolved: facebookincubator#11298

Reviewed By: Yuhta

Differential Revision: D65834210

Pulled By: kevinwilfong

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

@Yuhta Yuhta Yuhta approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SelectiveDecimalColumnReader::scanSpec_::valueHook_ may not be nullptr
4 participants
@NEUpanning @Yuhta @facebook-github-bot @kevinwilfong