Add GenericTransformFunction wrapper for simple ScalarFunctions #5440

KKcorps · 2020-05-25T08:12:52Z

Today, adding new TransformFunction is quite involved. Supporting simple static java functions requires one to write a lot of boiler plate code. This PR adds a generic Transform Function that can invoke any of the registered functions in FunctionRegistry.

This PR tests this new feature with StringFunctions.

kishoreg

This is awesome! Change the description to say that this is a generic transform function that can call any scalarfunction that return a single value

...rc/main/java/org/apache/pinot/core/operator/transform/function/GenericTransformFunction.java

kishoreg · 2020-05-25T08:23:14Z

...rc/main/java/org/apache/pinot/core/operator/transform/function/GenericTransformFunction.java

+
+  @Override
+  public TransformResultMetadata getResultMetadata() {
+    return STRING_SV_NO_DICTIONARY_METADATA;


you can add another switch statement in init to set this based on the return type of the scalarfunction, see functionInvoker.getReturnType

For return type just a switch won't work
Different return types will need a different function overrides
e.g. transformToStringValuesSV for string, transformToIntValuesSV for int and so on
Or am I missing something?

You are right, we need to override methods for int, long, double, float. Shouldn't be hard, its mostly copy-paste. similar to _stringResultArray, create a resultArray for each type and instantiate them in the init

does it look better now?

xiangfu0 · 2020-05-25T09:17:03Z

This is great!
Could you add tests in: CalciteSqlCompilerTest to check sql parsing logic.

...rc/main/java/org/apache/pinot/core/operator/transform/function/TransformFunctionFactory.java

siddharthteotia · 2020-05-25T21:25:39Z

...est/java/org/apache/pinot/core/operator/transform/function/GenericTransformFunctionTest.java

+
+public class GenericTransformFunctionTest extends BaseTransformFunctionTest {
+
+  @Test


I believe this exercises the compilation path. You should add tests in CalciteSqlCompiler test file as well.

Also, we should add tests for end to end query execution. See TransformQueriesTest class or consider adding execution tests to one of the existing query tests

KKcorps · 2020-05-26T15:13:11Z

@fx19880617 @siddharthteotia Should I add tests in CalciteSQL for all the functions?

...rc/main/java/org/apache/pinot/core/operator/transform/function/GenericTransformFunction.java

...rc/main/java/org/apache/pinot/core/operator/transform/function/TransformFunctionFactory.java

pinot-common/pom.xml

pinot-common/src/main/java/org/apache/pinot/common/function/StringFunctions.java

pinot-common/src/main/java/org/apache/pinot/common/function/annotations/ScalarFunction.java

siddharthteotia · 2020-05-29T21:19:16Z

@fx19880617 @siddharthteotia Should I add tests in CalciteSQL for all the functions?

@KKcorps , sorry missed this. Yes, the query compilation tests should be in CalciteSqlCompilerTest. Here we can verify that PinotQuery is being built correctly and that gets converted to BrokerRequest correctly. Most other tests in this file do this validation.

The other suggestion was to also add unit tests for exercising end-to-end execution path. Please consider adding these tests to an appropriate file in /incubator-pinot/pinot-core/src/test/java/org/apache/pinot/queries/. May be TransformQueriesTest

…tion

pinot-common/pom.xml

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java

...n/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java

Jackie-Jiang · 2020-05-30T00:06:32Z

...n/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java

+      TransformFunction function = arguments.get(i);
+      if (function instanceof LiteralTransformFunction) {
+        String literal = ((LiteralTransformFunction) function).getLiteral();
+        Class paramType = _functionInvoker.getParameterTypes()[i];


Suggest directly comparing class instead of String comparison for both performance and readability:

if (paramType == Integer.class) { ... } else if (paramType == Long.class) { ...

It would require changing switch case to a lot of else if since switch doesn't accept Class type. IMO, switch case looks neat so made this trade off

...n/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java

...rc/main/java/org/apache/pinot/core/operator/transform/function/TransformFunctionFactory.java

...n/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java

Jackie-Jiang

Great feature! LGTM except for the unaddressed comments

siddharthteotia · 2020-06-02T23:58:25Z

Is the plan to use this wrapper solely for invoking scalar functions (like already done in this PR for StringFunctions) or are we expecting follow-ups to integrate it with rest of the transform functions. I think only the former?

kishoreg · 2020-06-03T01:20:08Z

@sidd only for scalarfunctions.

The new FunctionRegistry change of using reflection to plug in methods (introduced in #5440) is causing failure in query compilation. The problem is caused by Reflections library not being thread-safe when multiple threads are accessing the same jar file. We have multiple libraries (jersey, swagger) using reflection to load classes. The FunctionRegistry uses reflection in the static block, which happens when the class is loaded. If the first query arrives (first usage of FunctionRegistry which runs the static block) during the setup of the jersey server, Reflections will throw exception. Using reflection in the static block can also cause the first query to block on reflection scanning the classes, which can potentially take seconds. Read more about the thread-safety issue here: ronmamo/reflections#81 Users are reporting the same issue even with the latest Reflections version 0.9.12. A common solution is to downgrade Reflections to 0.9.9, but we have other dependencies rely on 0.9.11, so downgrading might cause other issues. The solution here is to introduce a no-op init() method to trigger the static block so that we can control when to scan the files to avoid the thread-safety issue as well as blocking the first query. Another benefit of using an init() method is that the exception won't be swallowed (the exception in static block will be swallowed and query engine will start getting ClassNotFoundException) This PR also introduces a convention for the plugin methods using reflection, where the class must includes ".function." in its class path. This can significantly reduce the time of class scanning (reduced from 4 seconds to 200 ms locally)

kishoreg reviewed May 25, 2020

View reviewed changes

siddharthteotia reviewed May 25, 2020

View reviewed changes

...rc/main/java/org/apache/pinot/core/operator/transform/function/TransformFunctionFactory.java Show resolved Hide resolved

siddharthteotia reviewed May 25, 2020

View reviewed changes

kishoreg changed the title ~~Add UDFs for String Transformation~~ Add GenericTransformFunction wrapper for simple ScalarFunctions May 26, 2020

kishoreg reviewed May 26, 2020

View reviewed changes

kishoreg approved these changes May 28, 2020

View reviewed changes

pinot-common/pom.xml Outdated Show resolved Hide resolved

pinot-common/src/main/java/org/apache/pinot/common/function/StringFunctions.java Show resolved Hide resolved

pinot-common/src/main/java/org/apache/pinot/common/function/annotations/ScalarFunction.java Show resolved Hide resolved

kishoreg and others added 8 commits May 30, 2020 04:03

Adding support to invoke any scalar function via GenericTransformFunc…

1aee329

…tion

string functions

05f5c69

adding string functions along with tests

85d2753

adding support for multiple types

58a16ca

refactoring

4600630

adding scalar function annotation

bf2fa38

refactoring and adding javadoc

20a099c

adding pom property

90f3854

KKcorps force-pushed the add_udf branch from 3eee657 to 90f3854 Compare May 29, 2020 22:41

Jackie-Jiang reviewed May 30, 2020

View reviewed changes

Kartik Khare added 4 commits June 2, 2020 01:13

moving pom version to root and refactoring

0404e96

fixing tests

9a21b54

refactoring: adding constructor to wrapper class

d116125

moving dependency out of module

4edaed4

kishoreg approved these changes Jun 2, 2020

View reviewed changes

Jackie-Jiang approved these changes Jun 2, 2020

View reviewed changes

siddharthteotia approved these changes Jun 2, 2020

View reviewed changes

kishoreg merged commit f7417ff into apache:master Jun 3, 2020

Jackie-Jiang mentioned this pull request Jun 9, 2020

Fix the failure caused by Reflections in FunctionRegistry #5531

Merged

1 task

Jackie-Jiang mentioned this pull request Aug 20, 2023

Add string udfs #4860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GenericTransformFunction wrapper for simple ScalarFunctions #5440

Add GenericTransformFunction wrapper for simple ScalarFunctions #5440

KKcorps commented May 25, 2020 •

edited by kishoreg

Loading

kishoreg left a comment

kishoreg May 25, 2020

KKcorps May 25, 2020

kishoreg May 25, 2020

KKcorps May 26, 2020

xiangfu0 commented May 25, 2020

siddharthteotia May 25, 2020 •

edited

Loading

KKcorps commented May 26, 2020 •

edited by siddharthteotia

Loading

siddharthteotia commented May 29, 2020 •

edited

Loading

Jackie-Jiang May 30, 2020

KKcorps Jun 1, 2020 •

edited

Loading

Jackie-Jiang left a comment

siddharthteotia commented Jun 2, 2020

kishoreg commented Jun 3, 2020


		public class GenericTransformFunctionTest extends BaseTransformFunctionTest {

		@Test

Add GenericTransformFunction wrapper for simple ScalarFunctions #5440

Add GenericTransformFunction wrapper for simple ScalarFunctions #5440

Conversation

KKcorps commented May 25, 2020 • edited by kishoreg Loading

kishoreg left a comment

Choose a reason for hiding this comment

kishoreg May 25, 2020

Choose a reason for hiding this comment

KKcorps May 25, 2020

Choose a reason for hiding this comment

kishoreg May 25, 2020

Choose a reason for hiding this comment

KKcorps May 26, 2020

Choose a reason for hiding this comment

xiangfu0 commented May 25, 2020

siddharthteotia May 25, 2020 • edited Loading

Choose a reason for hiding this comment

KKcorps commented May 26, 2020 • edited by siddharthteotia Loading

siddharthteotia commented May 29, 2020 • edited Loading

Jackie-Jiang May 30, 2020

Choose a reason for hiding this comment

KKcorps Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

siddharthteotia commented Jun 2, 2020

kishoreg commented Jun 3, 2020

KKcorps commented May 25, 2020 •

edited by kishoreg

Loading

siddharthteotia May 25, 2020 •

edited

Loading

KKcorps commented May 26, 2020 •

edited by siddharthteotia

Loading

siddharthteotia commented May 29, 2020 •

edited

Loading

KKcorps Jun 1, 2020 •

edited

Loading