Add from_ieee754_32 Presto function #7903

pdabre12 · 2023-12-06T20:55:57Z

Reference: https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/scalar/VarbinaryFunctions.java#L267

netlify · 2023-12-06T20:56:03Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`0d8a91a`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6605bdd759471600086577c8

aditi-pandit

Thanks @pdabre12.

Code looks good. Small comment in the tests.

velox/functions/prestosql/tests/BinaryFunctionsTest.cpp

aditi-pandit · 2024-02-01T03:14:15Z

@pdabre12 : Please can you add results of ExpressionFuzzer with this function https://facebookincubator.github.io/velox/develop/testing/fuzzer.html#how-to-run

pdabre12 · 2024-02-01T19:48:26Z

This test fails because

Expected equality of these values:

std::numeric_limits<float>::quiet_NaN()
    Which is: nan
  fromIEEE754Bits32( toIEEE754Bits32(std::numeric_limits<float>::quiet_NaN()))
  Which is: (nan)

Any suggestions of what I can do about the parenthesis () surrounding nan?

czentgr · 2024-02-02T22:49:56Z

This test fails because

Expected equality of these values:
std::numeric_limits<float>::quiet_NaN()
    Which is: nan
  fromIEEE754Bits32( toIEEE754Bits32(std::numeric_limits<float>::quiet_NaN()))
  Which is: (nan)
Any suggestions of what I can do about the parenthesis () surrounding nan?

As discussed. You can't compare using EXPECT_EQ because this is the same as nan == nan which always returns false ("A NaN never compares equal to itself." from the cpp reference). Instead, check that the return from the function call is actually a nan via std::isnan and EXPECT_TRUE.

aditi-pandit

Thanks @pdabre12 . Code is good.

aditi-pandit

@pdabre12 : The linux-build has test failures. Please fix the tests as per Christian's comments.

pdabre12 · 2024-02-06T02:13:10Z

@aditi-pandit Please review. The pipeline is clean now.

pdabre12 · 2024-02-06T02:19:41Z

Fuzzer output:

I20240205 18:16:46.861337 22035276 ExpressionVerifier.cpp:153] Starting simplified eval execution.
I20240205 18:16:46.861505 22035276 FuzzerToolkit.cpp:103] Exceptions match.
I20240205 18:16:46.861514 22035276 ExpressionFuzzerVerifier.cpp:363] ==============================> Done with iteration 39674
I20240205 18:16:46.861541 22035276 ExpressionFuzzerVerifier.cpp:160] ==============================> Top 1 by number of rows processed
I20240205 18:16:46.861547 22035276 ExpressionFuzzerVerifier.cpp:162] Format: functionName numTimesSelected proportionOfTimesSelected numProcessedRows
I20240205 18:16:46.861552 22035276 ExpressionFuzzerVerifier.cpp:166] from_ieee754_32 39675 100.00% 2896347
I20240205 18:16:46.861565 22035276 ExpressionFuzzerVerifier.cpp:172] ==============================> Bottom 1 by number of rows processed
I20240205 18:16:46.861572 22035276 ExpressionFuzzerVerifier.cpp:174] Format: functionName numTimesSelected proportionOfTimesSelected numProcessedRows
I20240205 18:16:46.861577 22035276 ExpressionFuzzerVerifier.cpp:179] from_ieee754_32 39675 100.00% 2896347
I20240205 18:16:46.861582 22035276 ExpressionFuzzerVerifier.cpp:192] ==============================> All stats sorted by number of times the function was chosen
I20240205 18:16:46.861588 22035276 ExpressionFuzzerVerifier.cpp:194] Format: functionName numTimesSelected proportionOfTimesSelected numProcessedRows
I20240205 18:16:46.861593 22035276 ExpressionFuzzerVerifier.cpp:198] from_ieee754_32 39675 100.00% 2896347
E20240205 18:16:46.861600 22035276 ExpressionFuzzerVerifier.cpp:369] Total iterations: 39675
E20240205 18:16:46.861605 22035276 ExpressionFuzzerVerifier.cpp:370] Total failed: 32138
[==========] Running 0 tests from 0 test suites.
[==========] 0 tests from 0 test suites ran. (0 ms total)
[  PASSED  ] 0 tests.

aditi-pandit

Thanks

aditi-pandit · 2024-02-06T05:35:06Z

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

ashokku2022 · 2024-02-26T20:26:22Z

@Yuhta @kagamiori I know you are busy with other high-priority issues, however, I was wondering if you could look into this one whenever you get time so that we can close it. Thanks!

aditi-pandit

@pdabre12 : Had a quick review comment.

aditi-pandit · 2024-02-29T21:30:49Z

velox/docs/functions/presto/binary.rst

@@ -94,6 +94,10 @@ Binary Functions

    Encodes ``real`` in a 32-bit big-endian binary according to IEEE 754 single-precision floating-point format.

+.. function:: from_ieee754_32(binary) -> real


Please move this to the right alphabetical order in the list.

aditi-pandit · 2024-02-29T21:33:53Z

velox/functions/prestosql/BinaryFunctions.h

+      out_type<float>& result,
+      const arg_type<Varbinary>& input) {
+    static constexpr auto kTypeLength = sizeof(int32_t);
+    VELOX_USER_CHECK_EQ(input.size(), kTypeLength, "Expected 4-byte input");


The java error is "Input floating-point value must be exactly 4 bytes long"

velox/functions/prestosql/tests/BinaryFunctionsTest.cpp

aditi-pandit

Please see comments above.

pdabre12 · 2024-03-01T03:03:34Z

@aditi-pandit Addressed the comments, please review again.
Thanks.

aditi-pandit

Thanks @pdabre12

aditi-pandit · 2024-03-01T16:28:13Z

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

facebook-github-bot · 2024-03-27T20:16:50Z

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

velox/docs/functions/presto/binary.rst

facebook-github-bot · 2024-03-28T19:15:28Z

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

facebook-github-bot · 2024-03-28T23:19:50Z

@Yuhta merged this pull request in 2d899d4.

conbench-facebook · 2024-03-28T23:42:17Z

Conbench analyzed the 1 benchmark run on commit 2d899d4f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Add from_ieee754_32() scalar function Resolves : facebookincubator#4427 Reference: https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/scalar/VarbinaryFunctions.java#L267 Pull Request resolved: facebookincubator#7903 Reviewed By: mbasmanova Differential Revision: D55440664 Pulled By: Yuhta fbshipit-source-id: 691532afbdc053c357d9a9d6241701df69dde682

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 6, 2023

pdabre12 force-pushed the add-from_ieee754_32 branch from b063468 to 82678ad Compare January 5, 2024 10:10

pdabre12 force-pushed the add-from_ieee754_32 branch from 82678ad to b8a6db1 Compare January 29, 2024 18:25

aditi-pandit reviewed Feb 1, 2024

View reviewed changes

velox/functions/prestosql/tests/BinaryFunctionsTest.cpp Show resolved Hide resolved

pdabre12 force-pushed the add-from_ieee754_32 branch 2 times, most recently from 43a3ce1 to e2093e2 Compare February 1, 2024 06:01

majetideepak requested a review from czentgr February 1, 2024 17:36

aditi-pandit approved these changes Feb 5, 2024

View reviewed changes

aditi-pandit requested changes Feb 5, 2024

View reviewed changes

pdabre12 force-pushed the add-from_ieee754_32 branch 3 times, most recently from 8ff4883 to 4df9d68 Compare February 5, 2024 23:24

aditi-pandit approved these changes Feb 6, 2024

View reviewed changes

aditi-pandit requested review from Yuhta and kagamiori February 6, 2024 05:34

aditi-pandit reviewed Feb 29, 2024

View reviewed changes

velox/functions/prestosql/tests/BinaryFunctionsTest.cpp Show resolved Hide resolved

aditi-pandit requested changes Feb 29, 2024

View reviewed changes

pdabre12 force-pushed the add-from_ieee754_32 branch from 4df9d68 to 7f6724e Compare March 1, 2024 00:05

aditi-pandit approved these changes Mar 1, 2024

View reviewed changes

Yuhta reviewed Mar 28, 2024

View reviewed changes

velox/docs/functions/presto/binary.rst Show resolved Hide resolved

pdabre12 force-pushed the add-from_ieee754_32 branch 2 times, most recently from 0d9494e to 7a6ec4c Compare March 28, 2024 17:28

Add from_ieee754_32 Presto scalar function

0d8a91a

pdabre12 force-pushed the add-from_ieee754_32 branch from 7a6ec4c to 0d8a91a Compare March 28, 2024 18:58

mbasmanova changed the title ~~Add from_ieee754_32() Presto function~~ Add from_ieee754_32 Presto function Mar 28, 2024

facebook-github-bot closed this in 2d899d4 Mar 28, 2024

facebook-github-bot added the Merged label Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add from_ieee754_32 Presto function #7903

Add from_ieee754_32 Presto function #7903

pdabre12 commented Dec 6, 2023 •

edited by mbasmanova

Loading

netlify bot commented Dec 6, 2023 •

edited

Loading

aditi-pandit left a comment

aditi-pandit commented Feb 1, 2024

pdabre12 commented Feb 1, 2024

czentgr commented Feb 2, 2024

aditi-pandit left a comment

aditi-pandit left a comment

pdabre12 commented Feb 6, 2024

pdabre12 commented Feb 6, 2024

aditi-pandit left a comment

aditi-pandit commented Feb 6, 2024

ashokku2022 commented Feb 26, 2024

aditi-pandit left a comment

aditi-pandit Feb 29, 2024

aditi-pandit Feb 29, 2024

aditi-pandit left a comment

pdabre12 commented Mar 1, 2024

aditi-pandit left a comment

aditi-pandit commented Mar 1, 2024

facebook-github-bot commented Mar 27, 2024

facebook-github-bot commented Mar 28, 2024

facebook-github-bot commented Mar 28, 2024

conbench-facebook bot commented Mar 28, 2024

		@@ -94,6 +94,10 @@ Binary Functions

		Encodes ``real`` in a 32-bit big-endian binary according to IEEE 754 single-precision floating-point format.

		.. function:: from_ieee754_32(binary) -> real

Add from_ieee754_32 Presto function #7903

Add from_ieee754_32 Presto function #7903

Conversation

pdabre12 commented Dec 6, 2023 • edited by mbasmanova Loading

netlify bot commented Dec 6, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit commented Feb 1, 2024

pdabre12 commented Feb 1, 2024

czentgr commented Feb 2, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

pdabre12 commented Feb 6, 2024

pdabre12 commented Feb 6, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit commented Feb 6, 2024

ashokku2022 commented Feb 26, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit Feb 29, 2024

Choose a reason for hiding this comment

aditi-pandit Feb 29, 2024

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

pdabre12 commented Mar 1, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit commented Mar 1, 2024

facebook-github-bot commented Mar 27, 2024

facebook-github-bot commented Mar 28, 2024

facebook-github-bot commented Mar 28, 2024

conbench-facebook bot commented Mar 28, 2024

pdabre12 commented Dec 6, 2023 •

edited by mbasmanova

Loading

netlify bot commented Dec 6, 2023 •

edited

Loading