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

Adding long decimal support for arbitrary() function #9217

Closed
wants to merge 4 commits into from
Closed

Adding long decimal support for arbitrary() function #9217

wants to merge 4 commits into from

Conversation

minhancao
Copy link
Contributor

Delivers #7311

@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 Mar 22, 2024
@netlify Netlify
Copy link

netlify bot commented Mar 22, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox ready!

Name Link
🔨 Latest commit be7a9ee
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/661833857c78c7000827c223
😎 Deploy Preview https://deploy-preview-9217--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.

aditi-pandit
aditi-pandit approved these changes Mar 27, 2024
View reviewed changes
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 @minhancao

aditi-pandit
aditi-pandit requested changes Mar 27, 2024
View reviewed changes
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.

@minhancao : Please can you run AggregationFuzzer and post results from it as well https://facebookincubator.github.io/velox/develop/testing/fuzzer.html#aggregation-fuzzer

@minhancao
Copy link
Contributor Author

@aditi-pandit
Here is the result for the aggregation fuzzer test for arbitrary():

I ran _build/debug/velox/functions/prestosql/fuzzer/velox_aggregation_fuzzer_test --seed 123456 --duration_sec 60 --logtostderr=1 --minloglevel=0 --only arbitrary --v=1 2>&1 | tee agg_fuzzer_test_arbitrary.out

I can provide the whole output file if needed.

I20240327 10:43:07.766820 4177649 AggregationFuzzer.cpp:453] ==============================> Done with iteration 22
I20240327 10:43:07.766849 4177649 AggregationFuzzer.cpp:992] Total masked aggregations: 3 (13.04%)
I20240327 10:43:07.766871 4177649 AggregationFuzzer.cpp:994] Total global aggregations: 0 (0.00%)
I20240327 10:43:07.766885 4177649 AggregationFuzzer.cpp:996] Total group-by aggregations: 20 (86.96%)
I20240327 10:43:07.766901 4177649 AggregationFuzzer.cpp:998] Total distinct aggregations: 3 (13.04%)
I20240327 10:43:07.766918 4177649 AggregationFuzzer.cpp:1000] Total aggregations over distinct inputs: 3 (13.04%)
I20240327 10:43:07.766933 4177649 AggregationFuzzer.cpp:1002] Total window expressions: 0 (0.00%)
I20240327 10:43:07.766947 4177649 AggregationFuzzerBase.cpp:557] Total functions tested: 1
I20240327 10:43:07.766960 4177649 AggregationFuzzerBase.cpp:558] Total iterations requiring sorted inputs: 3 (13.04%)
I20240327 10:43:07.766974 4177649 AggregationFuzzerBase.cpp:560] Total iterations verified against reference DB: 4 (17.39%)
I20240327 10:43:07.766988 4177649 AggregationFuzzerBase.cpp:562] Total functions not verified (verification skipped / not supported by reference DB / reference DB failed): 17 (73.91%) / 2 (8.70%) / 0 (0.00%)
I20240327 10:43:07.767002 4177649 AggregationFuzzerBase.cpp:567] Total failed functions: 0 (0.00%)
[==========] Running 0 tests from 0 test suites.
[==========] 0 tests from 0 test suites ran. (0 ms total)
[  PASSED  ] 0 tests.

aditi-pandit
aditi-pandit approved these changes Mar 27, 2024
View reviewed changes
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 @minhancao

@aditi-pandit aditi-pandit requested review from Yuhta and kagamiori March 27, 2024 20:28
@aditi-pandit
Copy link
Collaborator

@Yuhta @kagamiori : Please can you help with review and merge.

karteekmurthys
karteekmurthys approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@karteekmurthys karteekmurthys left a comment

Choose a reason for hiding this comment

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

Thanks @minhancao!

karteekmurthys
karteekmurthys reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@karteekmurthys karteekmurthys left a comment

Choose a reason for hiding this comment

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

[ RUN ] ArbitraryTest.longDecimal
*** Aborted at 1712092248 (Unix time, try 'date -d @1712092248') ***
*** Signal 11 (SIGSEGV) (0x0) received by PID 19932 (pthread TID 0x7f87f4bff700) (linux TID 57052) (code: 128), stack trace: ***
(error retrieving stack trace)

test 294
Start 294: velox_s3registration_test

Test failure. Likely we may have to switchto aligned memory allocator.

@minhancao
Copy link
Contributor Author

@karteekmurthys Thank you for identifying the test failure above! I was wondering how the switch to aligned memory allocator should be approached or implemented?

Would it be similar to what you have in your PR from here? https://github.com/facebookincubator/velox/pull/8723/files#diff-5091b1dd8232c357067be47fb0e751772f91a0aa12dea5db835a2d4ae296a63cR93

@karteekmurthys
Copy link
Contributor

@karteekmurthys Thank you for identifying the test failure above! I was wondering how the switch to aligned memory allocator should be approached or implemented?

Would it be similar to what you have in your PR from here? https://github.com/facebookincubator/velox/pull/8723/files#diff-5091b1dd8232c357067be47fb0e751772f91a0aa12dea5db835a2d4ae296a63cR93

Yes. likely. Please try it.

@minhancao minhancao force-pushed the add_long_decimal_support_for_arbitrary_oss branch from d1de7f1 to 74ed85b Compare April 10, 2024 19:16
@karteekmurthys
Copy link
Contributor

@minhancao would you please try overriding this method:

 int32_t accumulatorAlignmentSize() const override {
    return static_cast<int32_t>(sizeof(int128_t));
  }

@minhancao
Copy link
Contributor Author

@karteekmurthys I have implemented the change above, how do I get the linux CircleCI test to run on this PR?

@karteekmurthys
Copy link
Contributor

@kagamiori would you please review this.

@karteekmurthys
Copy link
Contributor

@Yuhta would you please help with the review.

@minhancao
Copy link
Contributor Author

minhancao commented Apr 22, 2024 •
edited
Loading

Hi @kagamiori @Yuhta , can you review this PR when you can please? Thanks!

kagamiori
kagamiori approved these changes Apr 27, 2024
View reviewed changes
Copy link
Contributor

@kagamiori kagamiori left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for adding this support!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@kagamiori merged this pull request in 84acb4c.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 84acb4c8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
@minhancao @Joe-Abraham
Adding long decimal support for arbitrary() function (facebookincubat…
d4cebaa 
…or#9217)

Summary:
Delivers facebookincubator#7311

Pull Request resolved: facebookincubator#9217

Reviewed By: amitkdutta

Differential Revision: D56662231

Pulled By: kagamiori

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

@karteekmurthys karteekmurthys karteekmurthys approved these changes

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@kagamiori kagamiori kagamiori approved these changes

@Yuhta Yuhta Awaiting requested review from Yuhta

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@minhancao @aditi-pandit @karteekmurthys @facebook-github-bot @kagamiori