Allow aliasing of Macros and add new alias for complex decode 64 #15034

pranavbhole · 2023-09-25T22:39:46Z

Description

This PR allows to build the macros/BuiltIn Macros with aliasing and also adds the alias for ComplexDecodeBase64OperatorConversion.

This PR has:

somu-imply · 2023-09-26T16:38:55Z

sql/src/main/java/org/apache/druid/sql/calcite/expression/SqlOperatorConversion.java

+   *
+   * @return operator
+   */
+  default Optional<SqlOperator> aliasCalciteOperator()


Why are not using the same technique as we do for TRUNC or SUBSTR by using the AliasedOperatorConversion ? (at least for the SQL side of things)

used AliasedOperatorConversion

somu-imply · 2023-09-26T16:47:57Z

processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java

 import java.util.stream.Collectors;

 public class BuiltInExprMacros
 {
  public static class ComplexDecodeBase64ExprMacro implements ExprMacroTable.ExprMacro
  {
    public static final String NAME = "complex_decode_base64";
+    public static final String ALIAS_NAME = "decode_base64_complex";


If we change this alias in the future is there a way to maintain backward compatibility?

Also, we might want to add native tests with the alias name to ensure the correctness

we can not change the alias name once defined else it will regression, we need to add more alias.
cool will add native test.

somu-imply

LGTM! Followup Q. Does this also allow using aliases in the native query? Although aliasing in native should be handled separately though and we can always address that as a followup

pranavbhole · 2023-10-03T01:18:57Z

LGTM! Followup Q. Does this also allow using aliases in the native query? Although aliasing in native should be handled separately though and we can always address that as a followup

Yes, we need to add alias at Native layer as well. #15034 (review) will make sure that Macro aliasing will work at native layer as well.

clintropolis · 2023-10-03T02:19:50Z

processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java


    @Override
    public String name()
    {
      return NAME;
    }

+    @Override
+    public Optional<String> alias()


I'm a bit worried this might be a bit too limiting since there can only be a single alias, unlike the SQL layer which can have any number of alias for the same function.

Additionally, I think there might be a bit more work to do here in terms of making the error messaging correct. Using this function as an example, the error messaging is going to be using the output of name() rather than the alias, so the error messaging for decode_base64_complex is going to be reporting complex_decode_base64, which I think might be confusing for users. We might want to consider doing aliasing similar to how it is done at the SQL layer, where we have some AliasNamedFunction class that overrides the name() method with the alias and then delegates everything else to the underlying expression which is being overridden, though it might take separate classes for ExprMacroFunctionExpr, FunctionExpr, and ApplyFunctionExpr. Depending on how these are registered with the function tables, this might also solve the multi-alias problem this current approach has, and future proofs against cases where we need to rename functions more than one time.

Thank you for review, pushed a commit to create AliasExprMacro. Please review it again.

processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java

somu-imply · 2023-10-05T01:53:55Z

processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java

+  /***
+   * Alias Expression macro create an alias and deligates operations to same base macro
+   */
+  public static class AliasExprMacro implements ExprMacroTable.ExprMacro


Is there a need to make this static? This can exist as a separate public class maybe?

moved to new public class

An internal class of ExprMacroTable would also probably be a more appropriate place for this to live so it is more obvious that functions defined by extensions could also use this

processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java

somu-imply · 2023-10-05T01:56:37Z

processing/src/test/java/org/apache/druid/math/expr/FunctionTest.java

@@ -952,6 +952,15 @@ public void testComplexDecode()
        ),
        expected
    );
+    // test with alias


Nit. Might be good to add a test with 2 aliases, just to drive the point home that we can support multiple aliases for the same

added test to verify multiple aliases

clintropolis

👍

clintropolis · 2023-10-05T05:58:01Z

...he/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregatorTest.java

+        );
+    testQuery(sql, expectedQueries, expectedResults);
+    testQuery(
+        StringUtils.replace(sql, "COMPLEX_DECODE_BASE64", "DECODE_BASE64_COMPLEX"),


nit: this seems kind of unnecessary to test both, maybe we should just update it to use DECODE_BASE64_COMPLEX since that is the new preferred function name

clintropolis · 2023-10-05T10:56:07Z

processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java

@@ -214,4 +224,31 @@ public Object getLiteralValue()
      }
    }
  }
+
+  /***
+   * Alias Expression macro to create an alias and delegate operations to the same base macro


javadocs on this should probably mention that for macros aliased by this thing, the Expr spit out by the apply method should use name() in all the places instead of an internal constant so that things like error messages behave correctly.

moved this class to internal class of ExprMacroTable and fixed javadoc

clintropolis · 2023-10-05T11:00:20Z

processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java

+  /***
+   * Alias Expression macro create an alias and deligates operations to same base macro
+   */
+  public static class AliasExprMacro implements ExprMacroTable.ExprMacro


An internal class of ExprMacroTable would also probably be a more appropriate place for this to live so it is more obvious that functions defined by extensions could also use this

…che#15034) * Add AliasExprMacro to allow aliasing of native expression macros * Add decode_base64_complex alias for complex_decode_base64

pranavbhole marked this pull request as ready for review September 25, 2023 22:39

github-actions bot added the Area - Querying label Sep 25, 2023

pranavbhole changed the title ~~Allow aliasing of SQL operator and Macros~~ Allow aliasing of SQL operators and Macros Sep 25, 2023

pranavbhole force-pushed the allowOperatorMacroAliasing branch from e65785a to 5dcbc0e Compare September 25, 2023 22:53

somu-imply reviewed Sep 26, 2023

View reviewed changes

pranavbhole added 2 commits October 1, 2023 22:48

Allow aliasing of SQL operator and Macros

1c4d649

Fix the NPE bug in nonStrictFormat

806433d

pranavbhole force-pushed the allowOperatorMacroAliasing branch 2 times, most recently from f0f5121 to e4fb4da Compare October 2, 2023 06:50

pranavbhole changed the title ~~Allow aliasing of SQL operators and Macros~~ Allow aliasing of Macros and add new alias for complex decode 64 Oct 2, 2023

somu-imply approved these changes Oct 3, 2023

View reviewed changes

using alias operator

8e24394

clintropolis reviewed Oct 3, 2023

View reviewed changes

pranavbhole force-pushed the allowOperatorMacroAliasing branch 2 times, most recently from 6b179e9 to 8c9e2a8 Compare October 5, 2023 00:59

somu-imply reviewed Oct 5, 2023

View reviewed changes

pranavbhole force-pushed the allowOperatorMacroAliasing branch 2 times, most recently from 090684d to d2d9589 Compare October 5, 2023 02:56

adding AliasExprMacro

7e02460

pranavbhole force-pushed the allowOperatorMacroAliasing branch from d2d9589 to 7e02460 Compare October 5, 2023 02:57

Adding test to verify aliases

56a8702

clintropolis reviewed Oct 5, 2023

View reviewed changes

addressed comments

db226cb

pranavbhole force-pushed the allowOperatorMacroAliasing branch from 287a78f to db226cb Compare October 5, 2023 16:37

clintropolis approved these changes Oct 5, 2023

View reviewed changes

clintropolis merged commit 06c5527 into apache:master Oct 5, 2023

pranavbhole deleted the allowOperatorMacroAliasing branch October 6, 2023 01:03

LakshSingla added this to the 28.0 milestone Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow aliasing of Macros and add new alias for complex decode 64 #15034

Allow aliasing of Macros and add new alias for complex decode 64 #15034

pranavbhole commented Sep 25, 2023 •

edited

Loading

somu-imply Sep 26, 2023

pranavbhole Oct 2, 2023

somu-imply Sep 26, 2023

pranavbhole Oct 2, 2023

somu-imply left a comment

pranavbhole commented Oct 3, 2023

clintropolis Oct 3, 2023

pranavbhole Oct 5, 2023

somu-imply Oct 5, 2023

pranavbhole Oct 5, 2023

clintropolis Oct 5, 2023

somu-imply Oct 5, 2023

pranavbhole Oct 5, 2023

clintropolis left a comment

clintropolis Oct 5, 2023

pranavbhole Oct 5, 2023

clintropolis Oct 5, 2023

pranavbhole Oct 5, 2023

clintropolis Oct 5, 2023

Allow aliasing of Macros and add new alias for complex decode 64 #15034

Allow aliasing of Macros and add new alias for complex decode 64 #15034

Conversation

pranavbhole commented Sep 25, 2023 • edited Loading

Description

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

somu-imply left a comment

Choose a reason for hiding this comment

pranavbhole commented Oct 3, 2023

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

clintropolis 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

pranavbhole commented Sep 25, 2023 •

edited

Loading