-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Rewrite Presto function greatest/least using simple function interface #3728
Comments
Hey @Yuhta, Could you assign this issue to me? Thanks, |
@tiagokepe Sure it's yours now |
@Yuhta is this issue resolved or still going on ? |
How can we register a simple function with The problem is that since PR #4434 we do not have At the same time
|
@mslapek No, we cannot register a simple function with |
@mslapek Simple Function API now supports decimal types. |
Hi, I am currently working on this issue if nobody is. It seems like this issue can be resolved after Simple Function API supports decimal types. Expected to give an update in one to two weeks. |
@Real-Chen-Happy For reference, decimal type support was added in #9096 |
Thank you! I just finished coding and all the existing test cases are passing. Before creating PR, may I know how to add a benchmark? I want to double check there is no performance degradation. |
@Real-Chen-Happy Sounds like you are making great progress. Check out velox/functions/prestosql/benchmarks/ArraySumBenchmark.cpp for an example of a benchmark and look at ExpressionBenchmarkBuilder as well as search the codebase for other usages of ExpressionBenchmarkBuilder. |
Got it! Are we expected to run benchmark on our local computer and compare the performance or it should be on some standard cloud instances? |
@Real-Chen-Happy For this change, I think running benchmarks is not necessary. When it is needed, we currently run on local computers of whatever hardware is available. It would be nice to standardize this a bit though. |
Makes sense! Thank you! |
Hi, PR is out #9308 |
@mbasmanova cc @s4ayub It seems like the simple function implementations are breaking existing internal UDF registration #9493 . May I know what are the issues and if there is anything I can fix on my end? |
Hi @Real-Chen-Happy , thank you for checking in. The internal issue was caused due to the switch to using Simple function interface instead of vector function. This changed the registration path that the internal product employed and caused unexpected behavior affecting production workloads. Therefore it was promptly rolled back. However, since the issue is isolated to the internal product and nothing needs to be rectified on velox, we will re-introduce the original velox change once it has been fixed internally which should be a couple of days at max. |
facebookincubator#9308) Summary: Refactor the greatest/least functions using simple function API. Also, add support for NaN comparisons for DOUBLE and REAL. NaN is the biggest according to prestodb/presto#22391 Fixes facebookincubator#3728 Pull Request resolved: facebookincubator#9308 Reviewed By: xiaoxmeng Differential Revision: D55793910 Pulled By: mbasmanova fbshipit-source-id: c389bad91197f00ced549d816a15efab5a2dd910
Description
Use the Variadic Arguments feature we can reimplement
greatest
/least
in Presto using simple function interface. Old code is at https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/GreatestLeast.cpp.Do not forget to add a benchmark to make sure no performance degradation.
The text was updated successfully, but these errors were encountered: