Adding new function decode_base64_utf8 and expr macro by pranavbhole · Pull Request #14943 · apache/druid

pranavbhole · 2023-09-06T02:23:45Z

Description

Adding a new function called decode_base64_utf8 that takes base64 encoded string, decode it and gives back the utf8 encoded string.

This PR has:

pranavbhole · 2023-09-06T02:26:21Z

meanwhile review, I am adding more tests.
Thank you

clintropolis · 2023-09-06T02:41:17Z

.../java/org/apache/druid/sql/calcite/expression/builtin/DecodeBase64UTFOperatorConversion.java

+
+import javax.annotation.Nullable;
+
+public class DecodeBase64UTFOperatorConversion implements SqlOperatorConversion


I think you can just use DirectOperatorConversion which will save you from having to implement toDruidExpression

clintropolis · 2023-09-06T02:43:18Z

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

+        if (toDecode.value() == null) {
+          return ExprEval.of(null);
+        }
+        return new StringExpr(new String(StringUtils.decodeBase64String(toDecode.asString()), StandardCharsets.UTF_8)).eval(bindings);


You could also use StringUtils.fromUtf8(StringUtils.decodeBase64String(...))

clintropolis · 2023-09-06T02:45:05Z

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

    }
  }
+
+  public static class StringDecodeBase64UTFExprMacro implements ExprMacroTable.ExprMacro


It would be useful to implement getOutputType, isLiteral, isNullLiteral, and getLiteralValue similar to ComplexDecodeBase64Expression.

General Q, can this be implemented as an extension of UnivariateFunction? But then this is an expression and not exactly a function

BaseScalarUnivariateMacroFunctionExpr is the macro version of right thing to extend here since this is a single arugment function, +1 for re-using it since it would simplify things a bit here

clintropolis · 2023-09-06T02:47:07Z

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

  private static final List<ExprMacro> BUILT_IN = ImmutableList.of(
-      new BuiltInExprMacros.ComplexDecodeBase64ExprMacro()
+      new BuiltInExprMacros.ComplexDecodeBase64ExprMacro(),
+      new BuiltInExprMacros.StringDecodeBase64UTFExprMacro()


it doesn't necessarily have to be in this PR, but it would be nice to add an alias for ComplexDecodeBase64ExprMacro (and also the SQL layer), perhaps just by extending the class and overriding the usage of NAME so it has consistent naming with this new function, e.g. decode_base64_complex

I think this can be done registering an AliasedOperatorConversion similar to how SUBSTR or TRUNC is done

that would fix the SQL layer, but this is native expression macros, which do not currently have a generic aliasing thing here.

@pranavbhole what are your plans for this aliasing? (I just want to make sure this actually gets done whether or not in this PR, and not lost)

I will add aliasing in the followup PR.

somu-imply

It'll be good to add

a test in CalciteQueryTest
Update website/.spelling with decode_base64_utf8

somu-imply · 2023-09-06T18:00:38Z

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

+    assertExpr("decode_base64_utf8('aGVsbG8=')", "hello");
+    assertExpr("decode_base64_utf8('V2hlbiBhbiBvbmlvbiBpcyBjdXQsIGNlcnRhaW4gKGxhY2hyeW1hdG9yKSBjb21wb3VuZHMgYXJlIHJlbGVhc2VkIGNhdXNpbmcgdGhlIG5lcnZlcyBhcm91bmQgdGhlIGV5ZXMgKGxhY3JpbWFsIGdsYW5kcykgdG8gYmVjb21lIGlycml0YXRlZC4=')", "When an onion is cut, certain (lachrymator) compounds are released causing the nerves around the eyes (lacrimal glands) to become irritated.");
+    assertExpr("decode_base64_utf8('eyJ0ZXN0IjogMX0=')", "{\"test\": 1}");
+    assertExpr("decode_base64_utf8('')", "");


nit. this can be null in SQL compatible mode

pranavbhole · 2023-09-11T18:02:24Z

addressed comments.

soumyava · 2023-09-11T23:53:36Z

LGTM. One question, is there a way say I have a column of encoded strings. Atm I understand the function works on string values. Not this PR but can we later try to make it work on a column too ? Will that be useful ? Something like SELECT DECODE_BASE64_UTF8("<column>") FROM <table>. Currently this throws an error like

SELECT DECODE_BASE64_UTF8("dim1") FROM foo

Error: RUNTIME_FAILURE (OPERATOR)
Illegal base64 character 2e
java.lang.IllegalArgumentException

So might want to think about a better error message. But need not be done in context of this PR

pranavbhole · 2023-09-12T01:28:40Z

LGTM. One question, is there a way say I have a column of encoded strings. Atm I understand the function works on string values. Not this PR but can we later try to make it work on a column too ? Will that be useful ? Something like SELECT DECODE_BASE64_UTF8("<column>") FROM <table>. Currently this throws an error like
SELECT DECODE_BASE64_UTF8("dim1") FROM foo

Error: RUNTIME_FAILURE (OPERATOR)
Illegal base64 character 2e
java.lang.IllegalArgumentException
So might want to think about a better error message. But need not be done in context of this PR

It works on the column as well, provided you have encoded base64 string value stored in column.
If you column value has any other non base64 chars then it will throw above error.

soumyava

LGTM, the aliasing will be done in a followup PR

clintropolis reviewed Sep 6, 2023

View reviewed changes

somu-imply reviewed Sep 6, 2023

View reviewed changes

pranavbhole force-pushed the decode_base64_utf8 branch 2 times, most recently from 601a87e to 491e4b5 Compare September 9, 2023 01:01

github-actions bot added Area - Querying Area - Ingestion labels Sep 9, 2023

pranavbhole force-pushed the decode_base64_utf8 branch 2 times, most recently from 040c898 to ad20520 Compare September 11, 2023 20:30

pranavbhole force-pushed the decode_base64_utf8 branch from ad20520 to ed64879 Compare September 18, 2023 22:10

github-actions bot removed the Area - Ingestion label Sep 18, 2023

pranavbhole added 4 commits September 20, 2023 14:26

Adding new function decode_base64_utf8 and expr macro

77e964b

using BaseScalarUnivariateMacroFunctionExpr

fa4d4b7

Print stack trace in case of debug in ChainedExecutionQueryRunner

dffab78

fix static check

304d973

pranavbhole force-pushed the decode_base64_utf8 branch from c592237 to 304d973 Compare September 20, 2023 21:26

soumyava approved these changes Sep 20, 2023

View reviewed changes

soumyava merged commit 883c269 into apache:master Sep 21, 2023

LakshSingla added this to the 28.0 milestone Oct 12, 2023

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed


		import javax.annotation.Nullable;

		public class DecodeBase64UTFOperatorConversion implements SqlOperatorConversion

Comments

Conversation

pranavbhole commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

pranavbhole commented Sep 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somu-imply Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavbhole Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somu-imply left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavbhole commented Sep 11, 2023

Uh oh!

soumyava commented Sep 11, 2023

Uh oh!

pranavbhole commented Sep 12, 2023

Uh oh!

soumyava left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pranavbhole commented Sep 6, 2023 •

edited

Loading

somu-imply Sep 6, 2023 •

edited

Loading

pranavbhole Sep 12, 2023 •

edited

Loading