Exposing optional replaceMissingValueWith in lookup function and macros by pranavbhole · Pull Request #14956 · apache/druid

pranavbhole · 2023-09-09T03:10:36Z

Description

Adding the optional 3rd argument replaceMissingValueWith to lookup extractor function that replaces the missing value from given lookups. Default value for replaceMissingValueWith is null. It is already supported in native layer. Just exposing this value to populated in SQL layer as well. On same line, updated the lookup macros to support replaceMissingValueWith.

Release note

Lookup function also accepts the 3rd argument called '$replaceMissingValueWith' as constant string, if you value is missing given lookups for queried key then lookup function return result value from replaceMissingValueWith eg:

LOOKUP(store, 'store_to_country', 'NA')

If value is missing from store_to_country lookup for given key 'store' then it will return 'NA'.

This PR has:

soumyava · 2023-09-09T19:06:22Z

 FROM sales
 GROUP BY 1
 ```
+Lookup function also accepts the 3rd argument called `$replaceMissingValueWith` as constant string, if you value is missing given lookups for queried key then lookup function return result value from `replaceMissingValueWith`


nit. The lookup function also accepts the 3rd argument called $replaceMissingValueWith as a constant string. If your value is missing a lookup for the queried key, the lookup function returns the result value from replaceMissingValueWith

soumyava · 2023-09-09T19:34:29Z

+    if (args.size() > 2) {
+      final Expr missingValExpr = args.get(2);
+      if (missingValExpr.isLiteral()) {
+        return missingValExpr.getLiteralValue().toString();


Q. when used in sqlCompatibleMode if there is a value like empty string should it be converted to null ?

Nope, regardless of the sqlCompatibleMode, replaceMissingValueWith is literal value passed by user and feature expect to replace it as it is.

clintropolis · 2023-09-12T00:01:10Z

  public Expr apply(final List<Expr> args)
  {
-    validationHelperCheckArgumentCount(args, 2);
+    validationHelperCheckMinArgumentCount(args, 2);


this should use maybe validationHelperCheckArgumentRange since it requires 2 or 3 arguments exactly

clintropolis · 2023-09-12T00:03:11Z

+        return missingValExpr.getLiteralValue().toString();
+      }
+    }
+    return null;


I think if you are not going to handle a dynamic argument for the 'missing value' parameter, this should be an exception if the argument is not a literal. There is validationHelperCheckArgIsLiteral that can be called if we only want to support this argument being a constant.

Silently returning null seems incorrect

added validationHelperCheckArgIsLiteral check

clintropolis · 2023-09-12T00:06:38Z

+The lookup function also accepts the 3rd argument called `replaceMissingValueWith` as a constant string. If your value is missing a lookup for the queried key, the lookup function returns the result value from `replaceMissingValueWith`
+For example:
+```
+LOOKUP(store, 'store_to_country', 'NA')
+```
+If value is missing from `store_to_country` lookup for given key 'store' then it will return `NA`.


you should probably indicate that this optional 3rd argument must be a constant.

math-expr.md, sql-functions.md, and sql-scalar.md should also be updated since they all document this function signature

soumyava

LGTM

clintropolis · 2023-09-12T19:26:13Z

+  @Test
+  public void testTooFewArgs()
+  {
+    expectException(IllegalArgumentException.class, "Function[lookup] requires 2 to 3 arguments");


i suppose if the range is 1 we could change this messaging to be like 2 or 3 but we don't have to worry about that in this PR

I think Range 1, will also throw the same message.

clintropolis · 2023-09-12T19:27:46Z

+    if (args.size() > 2) {
+      final Expr missingValExpr = args.get(2);
+      validationHelperCheckArgIsLiteral(missingValExpr, "third argument");
+      return String.valueOf(missingValExpr.getLiteralValue());


hmm, sorry I didn't notice this earlier, I think this should use Evals.asString so that if the value is null it doesn't become "null" as in https://github.com/apache/druid/pull/14956/files#diff-989c5e92f71c63c48629a62588a78bec198184ff896791058509176dffe91338R54

fixed, retained the original expression here. also checking literal in macro and added test to make sure that null literalValue is passed correctly same as native layer.

clintropolis · 2023-09-12T19:29:38Z

+      if (missingValExpr.isLiteral()) {
+        return missingValExpr.getLiteralValue().toString();
+      }


this should maybe have a similar validation to the native layer to make sure that the argument doesn't silently become null in the event that the expression is null

Added a test in CalciteQueryTest to make sure that null is wired up correctly to native extraction fn.
modified the returned value to(String) missingValExpr.getLiteralValue()

clintropolis · 2023-09-12T19:30:31Z

+    if (inputExpressions.size() > 2) {
+      final Expr missingValExpr = plannerContext.parseExpression(inputExpressions.get(2).getExpression());
+      if (missingValExpr.isLiteral()) {
+        return missingValExpr.getLiteralValue().toString();


can this argument be null? (add a test with the argument as NULL if it parses correctly, ignore if not)

yes it can be null, newly added test validates the correct behavior.
Thank you

gianm · 2023-09-14T22:23:30Z

+                                     SqlTypeFamily.CHARACTER,
+                                     SqlTypeFamily.CHARACTER
+                                 )
+                                 .requiredOperandCount(2)


Since the lookup expr only supports a literal "missing value replacement", this should include .literalOperands(2) (meaning operand 2 must be a literal).

gianm · 2023-09-14T22:24:23Z

 |`CHARACTER_LENGTH(expr)`|Alias for `LENGTH`.|
 |`STRLEN(expr)`|Alias for `LENGTH`.|
-|`LOOKUP(expr, lookupName)`|Look up `expr` in a registered [query-time lookup table](lookups.md). Note that lookups can also be queried directly using the [`lookup` schema](sql.md#from).|
+|`LOOKUP(expr, lookupName, [replaceMissingValueWith])`|Look up `expr` in a registered [query-time lookup table](lookups.md). Note that lookups can also be queried directly using the [`lookup` schema](sql.md#from).|


Update the text to say what replaceMissingValueWith does, and to point out that it must be a constant.

gianm · 2023-09-14T22:25:50Z

+              FN_NAME,
+              arg.stringify(),
+              lookupExpr.stringify(),
+              replaceMissingValueWith


This doesn't handle escaping properly. replaceMissingValueWith might contain ' itself, or newlines, etc.

The simplest way to fix this is to keep the expr around and stringify() it here, like we do with lookupExpr (which is also required to be a literal).

clintropolis · 2023-10-02T20:06:04Z

 |format|format(pattern[, args...]) returns a string formatted in the manner of Java's [String.format](https://docs.oracle.com/javase/8/docs/api/java/lang/String.html#format-java.lang.String-java.lang.Object...-).|
 |like|like(expr, pattern[, escape]) is equivalent to SQL `expr LIKE pattern`|
-|lookup|lookup(expr, lookup-name) looks up expr in a registered [query-time lookup](../querying/lookups.md)|
+|lookup|lookup(expr, lookup-name[,replaceMissingValueWith]) looks up expr in a registered [query-time lookup](../querying/lookups.md)|


nit: should probably specify replaceMissingValueWith must be a constant

clintropolis · 2023-10-02T20:10:12Z

        false,
-        null,
+        replaceMissingValueWith != null && replaceMissingValueWith.isLiteral()
+        ? (String) replaceMissingValueWith.getLiteralValue()


I suppose since other types can also be literals it might be safer to use Evals.asString instead of casting

clintropolis · 2023-10-02T20:12:15Z

+    if (inputExpressions.size() > 2) {
+      final Expr missingValExpr = plannerContext.parseExpression(inputExpressions.get(2).getExpression());
+      if (missingValExpr.isLiteral()) {
+        return (String) missingValExpr.getLiteralValue();


otoh, this one is probably fine as is and doesn't need to use Evals.asString like the native layer should since the operand type checker specifies the 3rd argument as character type, though it would also probably be harmless

clintropolis · 2023-10-02T20:13:44Z

+                        .build()
+        ),
+        ImmutableList.of(
+            new Object[]{"Missing_Value", NullHandling.sqlCompatible() ? null : "", 5L},


nit: can use NullHandling.defaultStringValue()

Co-authored-by: Clint Wylie <cjwylie@gmail.com>

…os (apache#14956) * Exposing optional replaceMissingValueWith in lookup function and macros * args range validation * Updating docs * Addressing comments * Update docs/querying/sql-scalar.md Co-authored-by: Clint Wylie <cjwylie@gmail.com> * Update docs/querying/sql-functions.md Co-authored-by: Clint Wylie <cjwylie@gmail.com> * Addressing comments --------- Co-authored-by: Clint Wylie <cjwylie@gmail.com>

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

pranavbhole force-pushed the replaceMissingValueWithLookup branch 2 times, most recently from 6602841 to ff8b736 Compare September 9, 2023 04:48

github-actions Bot added Area - Metrics/Event Emitting Area - Dependencies labels Sep 9, 2023

pranavbhole force-pushed the replaceMissingValueWithLookup branch from ff8b736 to d39cd68 Compare September 9, 2023 04:56

github-actions Bot removed Area - Metrics/Event Emitting Area - Dependencies labels Sep 9, 2023

soumyava reviewed Sep 9, 2023

View reviewed changes

pranavbhole force-pushed the replaceMissingValueWithLookup branch 5 times, most recently from 72624ed to 0eb390c Compare September 11, 2023 18:56

clintropolis reviewed Sep 12, 2023

View reviewed changes

github-actions Bot removed the Area - Ingestion label Sep 12, 2023

pranavbhole force-pushed the replaceMissingValueWithLookup branch 3 times, most recently from c5ab145 to d112231 Compare September 12, 2023 06:43

soumyava approved these changes Sep 12, 2023

View reviewed changes

clintropolis reviewed Sep 12, 2023

View reviewed changes

gianm reviewed Sep 14, 2023

View reviewed changes

pranavbhole added 3 commits September 30, 2023 22:03

Exposing optional replaceMissingValueWith in lookup function and macros

69a168d

args range validation

cf20919

Updating docs

757d8ac

pranavbhole force-pushed the replaceMissingValueWithLookup branch from d112231 to 3fbd640 Compare October 1, 2023 05:03

Addressing comments

bd0cac9

pranavbhole force-pushed the replaceMissingValueWithLookup branch from 3fbd640 to bd0cac9 Compare October 1, 2023 05:49

clintropolis approved these changes Oct 2, 2023

View reviewed changes

pranavbhole and others added 3 commits October 2, 2023 14:23

Update docs/querying/sql-scalar.md

fad8d27

Co-authored-by: Clint Wylie <cjwylie@gmail.com>

Update docs/querying/sql-functions.md

51aa863

Co-authored-by: Clint Wylie <cjwylie@gmail.com>

Addressing comments

03f0ff5

clintropolis merged commit f1edd67 into apache:master Oct 3, 2023

pranavbhole deleted the replaceMissingValueWithLookup branch October 3, 2023 01:20

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

Conversation

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

Uh oh!

Description

Release note

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!

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!

soumyava left a comment

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!

Choose a reason for hiding this comment

Uh oh!

clintropolis 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!

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!

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!

Uh oh!

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!

Reviewers

Assignees

Labels

pranavbhole commented Sep 9, 2023 •

edited

Loading

clintropolis Sep 12, 2023 •

edited

Loading