velox icon indicating copy to clipboard operation
velox copied to clipboard

Add Presto inverse_cauchy_cdf function

Open wills-feng opened this issue 1 year ago • 5 comments

Resolves https://github.com/facebookincubator/velox/issues/5301 This is a followup for https://github.com/facebookincubator/velox/pull/5341

wills-feng avatar Apr 15 '24 21:04 wills-feng

Deploy Preview for meta-velox canceled.

Name Link
Latest commit bcdf5293d9901a5f018e568e1244eef461677966
Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/668ccc8de277130008642acf

netlify[bot] avatar Apr 15 '24 21:04 netlify[bot]

@wills-feng : Please add a fuzzer run output.

aditi-pandit avatar Apr 16 '24 20:04 aditi-pandit

@aditi-pandit I rewrote the function and tests. Could you please review, thanks?

I20240422 12:18:39.055817 28617570 ExpressionFuzzerVerifier.cpp:367] ==============================> Done with iteration 174940
I20240422 12:18:39.055881 28617570 ExpressionFuzzerVerifier.cpp:164] ==============================> Top 1 by number of rows processed
I20240422 12:18:39.055895 28617570 ExpressionFuzzerVerifier.cpp:166] Format: functionName numTimesSelected proportionOfTimesSelected numProcessedRows
I20240422 12:18:39.055910 28617570 ExpressionFuzzerVerifier.cpp:170] inverse_cauchy_cdf 3969852 100.00% 4032571
I20240422 12:18:39.055929 28617570 ExpressionFuzzerVerifier.cpp:176] ==============================> Bottom 1 by number of rows processed
I20240422 12:18:39.055945 28617570 ExpressionFuzzerVerifier.cpp:178] Format: functionName numTimesSelected proportionOfTimesSelected numProcessedRows
I20240422 12:18:39.055961 28617570 ExpressionFuzzerVerifier.cpp:183] inverse_cauchy_cdf 3969852 100.00% 4032571
I20240422 12:18:39.055975 28617570 ExpressionFuzzerVerifier.cpp:196] ==============================> All stats sorted by number of times the function was chosen
I20240422 12:18:39.055992 28617570 ExpressionFuzzerVerifier.cpp:198] Format: functionName numTimesSelected proportionOfTimesSelected numProcessedRows
I20240422 12:18:39.056008 28617570 ExpressionFuzzerVerifier.cpp:202] inverse_cauchy_cdf 3969852 100.00% 4032571
E20240422 12:18:39.056025 28617570 ExpressionFuzzerVerifier.cpp:373] Total iterations: 174941
E20240422 12:18:39.056042 28617570 ExpressionFuzzerVerifier.cpp:374] Total failed: 11613
[==========] Running 0 tests from 0 test suites.
[==========] 0 tests from 0 test suites ran. (0 ms total)
[  PASSED  ] 0 tests.

wills-feng avatar Apr 22 '24 19:04 wills-feng

@wills-feng : Code looks good.

Though your release build has an error

velox/connectors/hive/storage_adapters/hdfs/tests/CMakeFiles/velox_hdfs_file_test.dir/HdfsFileSystemTest.cpp.o -MF velox/connectors/hive/storage_adapters/hdfs/tests/CMakeFiles/velox_hdfs_file_test.dir/HdfsFileSystemTest.cpp.o.d -o velox/connectors/hive/storage_adapters/hdfs/tests/CMakeFiles/velox_hdfs_file_test.dir/HdfsFileSystemTest.cpp.o -c /__w/velox/velox/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp
/__w/velox/velox/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp: In static member function 'static void HdfsFileSystemTest::SetUpTestSuite()':
/__w/velox/velox/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp:49:38: error: 'using element_type = class facebook::velox::exec::test::TempFilePath' {aka 'class facebook::velox::exec::test::TempFilePath'} has no member named 'path'; did you mean 'const string facebook::velox::exec::test::TempFilePath::path_'? (accessible via 'const string& facebook::velox::exec::test::TempFilePath::getPath() const')
   49 |       miniCluster->addFile(tempFile->path, destinationPath);
      |                                      ^~~~
      |                                      getPath()
[1473/1978] Building CXX object velox/connectors/hive/storage_adapters/hdfs/CMakeFiles/velox_hdfs.dir/HdfsWriteFile.cpp.o
[1474/1978] Building CXX object velox/connectors/hive/storage_adapters/hdfs/CMakeFiles/velox_hdfs.dir/RegisterHdfsFileSystem.cpp.o
[1475/1978] Building CXX object velox/connectors/hive/iceberg/tests/CMakeFiles/velox_hive_iceberg_test.dir/IcebergReadTest.cpp.o
[1476/1978] Building CXX object velox/connectors/hive/storage_adapters/s3fs/tests/CMakeFiles/velox_s3registration_test.dir/S3FileSystemRegistrationTest.cpp.o
[1477/1978] Building CXX object velox/connectors/hive/storage_adapters/hdfs/tests/CMakeFiles/velox_hdfs_file_test.dir/InsertIntoHdfsTest.cpp.o
[1478/1978] Building CXX object velox/connectors/hive/storage_adapters/s3fs/tests/CMakeFiles/velox_s3multiendpoints_test.dir/S3MultipleEndpointsTest.cpp.o
[1479/1978] Building CXX object velox/connectors/hive/storage_adapters/s3fs/tests/CMakeFiles/velox_s3insert_test.dir/S3InsertTest.cpp.o
ninja: build stopped: subcommand failed.
make[1]: *** [Makefile:103: build] Error 1
make[1]: Leaving directory '/__w/velox/velox'
make: *** [Makefile:110: release] Error 2

Maybe you need to rebase ?

Please let me know once you get all clean checks.

aditi-pandit avatar Apr 22 '24 19:04 aditi-pandit

Thanks @wills-feng.

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

These functions are from old PRs which were held back because of the boost issues found in the fuzzer runs. These are being moved ahead since boost is fixed and updated now.

aditi-pandit avatar Apr 28 '24 23:04 aditi-pandit

There are 3 probability functions PRs are waiting for merge. I have rebased https://github.com/facebookincubator/velox/pull/9492, once it gets merged, I will rebase this one.

wills-feng avatar Jul 08 '24 17:07 wills-feng

@wills-feng I just merged #9492. Could you go ahead and rebase this one?

pedroerp avatar Jul 09 '24 04:07 pedroerp

@pedroerp Thank you. Rebased this one. CI looks good.

wills-feng avatar Jul 09 '24 16:07 wills-feng

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

facebook-github-bot avatar Jul 10 '24 17:07 facebook-github-bot

@pedroerp merged this pull request in facebookincubator/velox@ba8d27da00d8eca253a64585ac86262783dd2803.

facebook-github-bot avatar Jul 11 '24 15:07 facebook-github-bot

Conbench analyzed the 1 benchmark run on commit ba8d27da.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

conbench-facebook[bot] avatar Jul 11 '24 16:07 conbench-facebook[bot]