[Coral-Hive] Extend VersionedSqlUserDefinedFunction to support version-specific function names #508

ljfgem · 2024-06-05T13:54:02Z

What changes are proposed in this pull request, and why are they necessary?

This PR is to support versioning UDF resolution for one unversioned UDF static registration.
For example, user can register com.linkedin.abc once and then use variations with standard versioning prefix in Hive view definitions. A UDF class is considered versioned if the prefix before the first dot follows coral_udf_version_(\d+|x)_(\d+|x)_(\d+|x) format.
To achieve the goal, we made the following changes:

modify HiveFunctionResolver to allow versioning UDF resolution while static registry only contains unversioned class name.
add functionClassName and getShortFunctionName() to VersionedSqlUserDefinedFunction.

After successful resolution in coral-hive, coral-spark would be able to return the UDF with versioning class name (functionClassName) so that Spark is able to register the versioning UDF class well. And coral-trino will use registered short name or fall back to convert the camel case class name to lowercase underscore function name (getShortFunctionName()).

How was this patch tested?

New unit tests in coral-hive, coral-spark and coral-trino, worked as expected.
Regression test on prod views.

wmoustafa · 2024-06-07T01:17:16Z

A UDF is considered shaded if the prefix before the first dot contains an underscore.

This convention is brittle.

Why is shading indicated by an underscore? Can we use something that utilizes patterns/templates or maybe suffixes? We will have to be explicit about how to resolve conflicts in case of multiple names/classes match.

By the way, resolving conflicts could happen in the "underscore" based approach, so it is not unique to the more generic solution suggested above. One resolution strategy is to fail loading the view if there is a collision.

We need a representation that does not open the door for too generic names like .* but also flexible enough to allow for minor things like version number.

ljfgem · 2024-06-07T14:09:36Z

A UDF is considered shaded if the prefix before the first dot contains an underscore.

This convention is brittle.

Why is shading indicated by an underscore? Can we use something that utilizes patterns/templates or maybe suffixes? We will have to be explicit about how to resolve conflicts in case of multiple names/classes match.

By the way, resolving conflicts could happen in the "underscore" based approach, so it is not unique to the more generic solution suggested above. One resolution strategy is to fail loading the view if there is a collision.

We need a representation that does not open the door for too generic names like .* but also flexible enough to allow for minor things like version number.

@wmoustafa The shading prefix resolution only happens for static registered UDFs in a_b.xyz format (must contain dot and the prefix before dot must contain underscore), so there won't be issue if we ensure that the future registered UDFs follow this pattern (no underscore in the prefix before dot).
But agreed that we can create a more deterministic and well specified template for our use.

wmoustafa

First high-level review.

wmoustafa · 2024-07-16T21:03:07Z

.../main/java/com/linkedin/coral/common/transformers/SourceOperatorMatchSqlCallTransformer.java

@@ -26,7 +28,7 @@ public SourceOperatorMatchSqlCallTransformer(String sourceOpName, int numOperand

  @Override
  protected boolean condition(SqlCall sqlCall) {
-    return sourceOpName.equalsIgnoreCase(sqlCall.getOperator().getName())
+    return sourceOpName.equalsIgnoreCase(FunctionUtils.removeShadingPrefix(sqlCall.getOperator().getName()))


I would not leak this shading business to a standard operator like this. Is there any way to handle this outside this operator?

Moved it to CoralRegistryOperatorRenameSqlCallTransformer in coral-trino.

wmoustafa · 2024-07-16T21:05:31Z

coral-common/src/main/java/com/linkedin/coral/common/utils/FunctionUtils.java

+
+  /**
+   * Checks if the given class name has a shading prefix.
+   * A class name is considered shaded if the prefix before the first dot


Can we remove references to shading and use versioning/versioned in the description and the code? Using shading terminology unnecessarily complicates the description.

Sure, modified.

wmoustafa · 2024-07-16T21:06:31Z

coral-common/src/main/java/com/linkedin/coral/common/utils/FunctionUtils.java

+
+
+public class FunctionUtils {
+  public static final String CORAL_VERSIONED_UDF_PREFIX = "coralversionedudf_(\\d+|x)_(\\d+|x)_(\\d+|x)";


coral_udf_version_..

wmoustafa · 2024-07-16T21:29:35Z

coral-common/src/main/java/com/linkedin/coral/common/utils/FunctionUtils.java

+  /**
+   * Generates a versioned function name based on the given function name and class name.
+   * For example, if the function name is "myFunction" and the class name is "coralversionedudf_1_0_0.com.linkedin.MyClass",
+   * the versioned function name will be "myFunction_1_0_0". If the class name is not shaded, such as "com.linkedin.MyClass",
+   * the versioned function name will be "myFunction".
+   */


Why is not this done at the view definition level? Sounds like we are adding too much use case specific code to the core Coral logic. I am guessing because doing it in the view will hurt Trino, but this is not a valid reason for adding custom code like this. FYI @phd3.

Also seeing the only use is in Spark (and the behavior is Spark specific), not sure if this fits in coral-common.

Moved it to VersionedSqlUserDefinedFunction.

wmoustafa · 2024-07-16T23:42:21Z

...rc/main/java/com/linkedin/coral/spark/transformers/FallBackToLinkedInHiveUDFTransformer.java

@@ -66,15 +67,15 @@ protected SqlCall transform(SqlCall sqlCall) {
      throw new UnsupportedUDFException(operatorName);
    }
    final String viewDependentFunctionName = operator.getViewDependentFunctionName();
+    String versionedFunctionName = FunctionUtils.getVersionedFunctionName(viewDependentFunctionName, operatorName);


Since we leverage VersionedSqlUserDefinedFunction jointly with this new code, is there a way to encapsulate all this functionality in this class?

Moved to a new method getVersionedViewDependentFunctionName in VersionedSqlUserDefinedFunction.

wmoustafa · 2024-08-20T22:31:13Z

coral-hive/src/main/java/com/linkedin/coral/hive/hive2rel/functions/HiveFunctionResolver.java

        // we want to see class name in the exception message for coverage testing
        // so throw exception here
        throw new UnknownSqlFunctionException(funcClassName);
      }

      return dynamicResolvedFunctions;
+    } else {
+      if (isVersioningUDF(funcClassName)) {


The logic here should be:

If it has the necessary properties (and hence Dali UDF)

If it is on the format [name]+_version or db_t_+[name], then

short name = [name].

function name = unversioned class name

long name = [name]+_version or db_t_+[name] or whatever original name is

else a special case of the above

Ideally Dali views should have function-identifier property that makes it possible to use this directly in the registry or in function name here and not derive function name from the class name but the latter is acceptable in the interim, especially everything is encapsulated in this method.

Also in case short name = [name] is not useful for Trino, we can have short name depend on function name through a hardcoded dependency. Intention of short name is to be used in engines that cannot leverage versioning like Trino.

Modified the code by adding a new method getTrinoFunctionName to VersionedSqlUserDefinedFunction, which returns the Trino function name as how we did before:

Try to retrieve the Trino function name from the registered map TRINO_FUNC_NAME_MAP based on the UDF class name

If the UDF class name doesn't exist in the map, follow the current approach by converting the camel case class name to lowercase underscore function name.

Since we don't have a standard for version in view function name (we only have a standard for versioning prefix in class name), and view function name may not be related to the registered Trino function name, I would prefer to keep the current approach of generating Trino function name, which would also avoid regressions.

wmoustafa · 2024-08-26T20:40:37Z

...rc/main/java/com/linkedin/coral/hive/hive2rel/functions/VersionedSqlUserDefinedFunction.java

+   * the short function name.
+   */
+  public String getShortFunctionName() {
+    String unversionedClassName = getName();


Please add java doc about the nature of the function name (e.g., its format etc). You can say this is just a convention and other naming approaches are still valid as long as they identify the type inference.

wmoustafa · 2024-08-26T20:43:51Z

coral-hive/src/test/java/com/linkedin/coral/hive/hive2rel/HiveToRelConverterTest.java

@@ -356,6 +356,14 @@ public void testDaliUDFCall() {
    assertEquals(RelOptUtil.toString(rel), expectedPlan);
  }

+  @Test
+  public void testUDFWithVersioningClassName() {


Please clarify the unit test name.

wmoustafa · 2024-08-26T20:45:53Z

coral-hive/src/main/java/com/linkedin/coral/hive/hive2rel/functions/HiveFunctionResolver.java

@@ -44,6 +44,7 @@
 * Class to resolve hive function names in SQL to Function.
 */
 public class HiveFunctionResolver {
+  private static final String CORAL_VERSIONED_UDF_PREFIX = "coral_udf_version_(\\d+|x)_(\\d+|x)_(\\d+|x)";


Remove CORAL_ since VERSIONED_UDF is clear enough that it is related to VersionedUDF class?
Should use CLASS_NAME_PREFIX?

wmoustafa · 2024-08-26T20:48:09Z

coral-hive/src/main/java/com/linkedin/coral/hive/hive2rel/functions/HiveFunctionResolver.java

@@ -218,7 +222,7 @@ public void addDynamicFunctionToTheRegistry(String funcClassName, Function funct
            new SqlUserDefinedFunction(new SqlIdentifier(funcClassName, ZERO),
                new HiveGenericUDFReturnTypeInference(funcClassName, hiveTable.getDaliUdfDependencies()), null,
                createSqlOperandTypeChecker(numOfOperands), null, null),
-            hiveTable.getDaliUdfDependencies(), functionName));
+            hiveTable.getDaliUdfDependencies(), functionName, funcClassName));


At some point we deal with functionName as a class name (e.g., String unversionedClassName = getName();). Is there a way to stick to one terminology througout the whole code?

Modified to use functionName to refer to the function name in view text, and funcClassName to refer to the function class string.

wmoustafa · 2024-08-26T20:48:49Z

...rc/main/java/com/linkedin/coral/hive/hive2rel/functions/VersionedSqlUserDefinedFunction.java

+  public String getUDFClassName() {
+    return udfClassName;
+  }


FunctionName vs UDF Name.. can we fix all terminology and unify them to minimize confusion?

coral-hive/src/main/java/com/linkedin/coral/hive/hive2rel/functions/HiveFunctionResolver.java

...rc/main/java/com/linkedin/coral/hive/hive2rel/functions/VersionedSqlUserDefinedFunction.java

coral-hive/src/main/java/com/linkedin/coral/hive/hive2rel/functions/HiveFunctionResolver.java

wmoustafa

Thank you so much @ljfgem for working on this. Looks great now. Could you please update the title and description to match the new approach/sematics? I do not "shading" is a thing anymore.

It is probably along the lines of Extend VersionedSqlUserDefinedFunction to support version-specific function names.

Please make sure Trino tests pass both regression and server side.

ljfgem · 2024-08-27T19:06:04Z

Thank you so much @ljfgem for working on this. Looks great now. Could you please update the title and description to match the new approach/sematics? I do not "shading" is a thing anymore.

It is probably along the lines of Extend VersionedSqlUserDefinedFunction to support version-specific function names.

Please make sure Trino tests pass both regression and server side.

@wmoustafa Thank you for your reviews and suggestions on this PR! Modified the title and description, also finished the required tests.

ljfgem force-pushed the support-shaded-udf branch from b61b0f3 to b1ab580 Compare July 11, 2024 17:51

wmoustafa reviewed Jul 16, 2024

View reviewed changes

ljfgem force-pushed the support-shaded-udf branch from 7d6a600 to 606473e Compare August 12, 2024 19:25

wmoustafa reviewed Aug 20, 2024

View reviewed changes

ljfgem force-pushed the support-shaded-udf branch from 0c9ba56 to 5372271 Compare August 21, 2024 15:18

Support versioning UDF

56e0e4d

ljfgem force-pushed the support-shaded-udf branch from 2e851af to 56e0e4d Compare August 22, 2024 20:03

ljfgem changed the title ~~[Coral-Hive] Support various shaded UDF resolution for one unshaded UDF static registration~~ [Coral-Hive] Support versioning UDF resolution for one unshaded UDF static registration Aug 22, 2024

ljfgem added 2 commits August 22, 2024 18:22

Address comments

9583bff

Add comments

d3d27d8

wmoustafa reviewed Aug 26, 2024

View reviewed changes

Address comments

aafd59d

wmoustafa reviewed Aug 26, 2024

View reviewed changes

Address comments

9e68ce2

wmoustafa approved these changes Aug 27, 2024

View reviewed changes

ljfgem changed the title ~~[Coral-Hive] Support versioning UDF resolution for one unshaded UDF static registration~~ [Coral-Hive] Extend VersionedSqlUserDefinedFunction to support version-specific function names Aug 27, 2024

ljfgem merged commit a8b86e7 into linkedin:master Aug 27, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Coral-Hive] Extend VersionedSqlUserDefinedFunction to support version-specific function names #508

[Coral-Hive] Extend VersionedSqlUserDefinedFunction to support version-specific function names #508

ljfgem commented Jun 5, 2024 •

edited

Loading

wmoustafa commented Jun 7, 2024

ljfgem commented Jun 7, 2024

wmoustafa left a comment

wmoustafa Jul 16, 2024

ljfgem Jul 18, 2024

wmoustafa Jul 16, 2024

ljfgem Jul 18, 2024

wmoustafa Jul 16, 2024

ljfgem Jul 18, 2024

wmoustafa Jul 16, 2024

wmoustafa Jul 16, 2024

ljfgem Jul 18, 2024 •

edited

Loading

wmoustafa Jul 16, 2024

ljfgem Jul 18, 2024

wmoustafa Aug 20, 2024 •

edited

Loading

wmoustafa Aug 20, 2024

ljfgem Aug 22, 2024 •

edited

Loading

wmoustafa Aug 26, 2024

ljfgem Aug 26, 2024

wmoustafa Aug 26, 2024

ljfgem Aug 26, 2024

wmoustafa Aug 26, 2024

ljfgem Aug 26, 2024

wmoustafa Aug 26, 2024

ljfgem Aug 26, 2024 •

edited

Loading

wmoustafa Aug 26, 2024

ljfgem Aug 26, 2024

wmoustafa left a comment

ljfgem commented Aug 27, 2024



		public class FunctionUtils {
		public static final String CORAL_VERSIONED_UDF_PREFIX = "coralversionedudf_(\\d+\|x)_(\\d+\|x)_(\\d+\|x)";

[Coral-Hive] Extend VersionedSqlUserDefinedFunction to support version-specific function names #508

[Coral-Hive] Extend VersionedSqlUserDefinedFunction to support version-specific function names #508

Conversation

ljfgem commented Jun 5, 2024 • edited Loading

What changes are proposed in this pull request, and why are they necessary?

How was this patch tested?

wmoustafa commented Jun 7, 2024

ljfgem commented Jun 7, 2024

wmoustafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljfgem Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmoustafa Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljfgem Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljfgem Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmoustafa left a comment

Choose a reason for hiding this comment

ljfgem commented Aug 27, 2024

ljfgem commented Jun 5, 2024 •

edited

Loading

ljfgem Jul 18, 2024 •

edited

Loading

wmoustafa Aug 20, 2024 •

edited

Loading

ljfgem Aug 22, 2024 •

edited

Loading

ljfgem Aug 26, 2024 •

edited

Loading