velox icon indicating copy to clipboard operation
velox copied to clipboard

Published 20 hours ago • verified publisher icon facebookincubator

Adding long decimal support for arbitrary() function

Open minhancao opened this issue 11 months ago • 4 comments

Delivers https://github.com/facebookincubator/velox/issues/7311

minhancao avatar Mar 22 '24 12:03 minhancao

Deploy Preview for meta-velox ready!

Name Link
Latest commit be7a9ee84710a53d9f2b1ebb7cf56c231f8a2301
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.

netlify[bot] avatar Mar 22 '24 12:03 netlify[bot]

@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.

minhancao avatar Mar 27 '24 19:03 minhancao

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

aditi-pandit avatar Mar 27 '24 20:03 aditi-pandit

@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

minhancao avatar Apr 03 '24 17:04 minhancao

@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.

karteekmurthys avatar Apr 04 '24 17:04 karteekmurthys

@minhancao would you please try overriding this method:

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

karteekmurthys avatar Apr 10 '24 21:04 karteekmurthys

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

minhancao avatar Apr 10 '24 21:04 minhancao

@kagamiori would you please review this.

karteekmurthys avatar Apr 11 '24 21:04 karteekmurthys

@Yuhta would you please help with the review.

karteekmurthys avatar Apr 15 '24 21:04 karteekmurthys

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

minhancao avatar Apr 22 '24 21:04 minhancao

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

facebook-github-bot avatar Apr 27 '24 01:04 facebook-github-bot

@kagamiori merged this pull request in facebookincubator/velox@84acb4c8d4bdf6f4dc74bfc22181852523a320f3.

facebook-github-bot avatar Apr 29 '24 17:04 facebook-github-bot

Conbench analyzed the 1 benchmark run on commit 84acb4c8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

conbench-facebook[bot] avatar Apr 29 '24 17:04 conbench-facebook[bot]