Rewrite Presto function greatest/least using simple function interface

[Yuhta]

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.

[tiagokepe]

Hey @Yuhta,

Could you assign this issue to me?

Thanks, Tiago.

[Yuhta]

@tiagokepe Sure it's yours now

[SANTHOSH-MAMIDISETTI]

@Yuhta is this issue resolved or still going on ? I am newbie to opensource and I would love to contribute , But I don't find any , it would be much appreciated if you may please help me find something or assign me something .

[mslapek]

How can we register a simple function with DECIMAL type?

The problem is that since PR https://github.com/facebookincubator/velox/pull/4434 we do not have Long/ShortDecimal... ☹️

At the same time greatest/least have a variant for decimals:

signatures.emplace_back(exec::FunctionSignatureBuilder()
                            .integerVariable("precision")
                            .integerVariable("scale")
                            .returnType("DECIMAL(precision, scale)")
                            .argumentType("DECIMAL(precision, scale)")
                            .variableArity()
                            .build());

[majetideepak]

@mslapek No, we cannot register a simple function with DECIMAL type. You have to write a Vector Function.

[mbasmanova]

@mslapek Simple Function API now supports decimal types.

[Real-Chen-Happy]

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.

[mbasmanova]

@Real-Chen-Happy For reference, decimal type support was added in https://github.com/facebookincubator/velox/pull/9096

[Real-Chen-Happy]

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.

[mbasmanova]

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

[Real-Chen-Happy]

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?

[mbasmanova]

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

[Real-Chen-Happy]

Makes sense! Thank you!

[Real-Chen-Happy]

Hi, PR is out https://github.com/facebookincubator/velox/pull/9308 Can somebody help review? Thank you!

[Real-Chen-Happy]

@mbasmanova cc @s4ayub It seems like the simple function implementations are breaking existing internal UDF registration https://github.com/facebookincubator/velox/pull/9493 . May I know what are the issues and if there is anything I can fix on my end?

[bikramSingh91]

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.