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.

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

Add Decimal support to set_agg and set_union #7936

Closed

Conversation

rrando901
Copy link
Contributor

Delivers #7935

Copy link

netlify bot commented Dec 8, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 77ae46b
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65cbfa383ce1aa0008dfdb2f

@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 Dec 8, 2023
velox/dwio/dwrf/writer/ColumnWriter.cpp Outdated Show resolved Hide resolved
velox/functions/prestosql/aggregates/SetAggregates.cpp Outdated Show resolved Hide resolved
velox/dwio/dwrf/writer/ColumnWriter.cpp Outdated Show resolved Hide resolved
velox/functions/prestosql/aggregates/SetAggregates.cpp Outdated Show resolved Hide resolved
velox/functions/prestosql/aggregates/SetAggregates.cpp Outdated Show resolved Hide resolved
velox/functions/prestosql/aggregates/tests/SetAggTest.cpp Outdated Show resolved Hide resolved
@rrando901 rrando901 force-pushed the setAggUnion-HugeInt branch 3 times, most recently from 5122717 to cedb8d6 Compare December 8, 2023 19:25
@rrando901 rrando901 force-pushed the setAggUnion-HugeInt branch from cedb8d6 to 09dbf26 Compare December 8, 2023 19:29
@ethanyzhang
Copy link

@aditi-pandit Can you help take a look?

Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @rrando901. One minor comment.

Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @rrando901

@aditi-pandit aditi-pandit requested a review from Yuhta January 9, 2024 03:55
@aditi-pandit
Copy link
Collaborator

@rrando901 : Your build has a fuzzer failure. Please can you rebase your code and try again. Also create an issue for the fuzzer failure if needed.

@rrando901 rrando901 force-pushed the setAggUnion-HugeInt branch from 829978c to 515dd6d Compare January 9, 2024 19:05
@rrando901
Copy link
Contributor Author

@aditi-pandit Rebased code with the latest from main. There is a fuzzer failure in AsyncDataCacheTest.cpp.

Looks like there are two issues opened for this already:

@rrando901
Copy link
Contributor Author

Hi @Yuhta. When you get a chance, could you take a look at these changes?

@rrando901 rrando901 force-pushed the setAggUnion-HugeInt branch from 515dd6d to 45fccbd Compare January 25, 2024 22:47
@rrando901
Copy link
Contributor Author

Rebased PR @aditi-pandit @Yuhta

@ethanyzhang
Copy link

@aditi-pandit please follow up, thanks!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@Yuhta merged this pull request in 615af51.

Copy link

Conbench analyzed the 1 benchmark run on commit 615af515.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants