Exposing optional replaceMissingValueWith in lookup function and macros #14956

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

docs/querying/lookups.md

@@ -62,6 +62,12 @@ SELECT
 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

processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java

+    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

processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java

@@ -54,10 +54,11 @@ public String name()
  @Override
  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

processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java

+        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

docs/querying/lookups.md

+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

processing/src/test/java/org/apache/druid/query/expression/LookupExprMacroTest.java

+  @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

processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java

+    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

...main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java

+      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

...main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java

+    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

...main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java

+                                     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

docs/querying/sql-scalar.md

@@ -98,7 +98,7 @@ String functions accept strings, and return a type appropriate to the function.
 |`CHAR_LENGTH(expr)`|Alias for `LENGTH`.|
 |`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

processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java

+              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

docs/querying/math-expr.md

@@ -82,7 +82,7 @@ The following built-in functions are available.
 |concat|concat(expr, expr...) concatenate a list of strings|
 |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

docs/querying/sql-functions.md

docs/querying/sql-scalar.md

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

processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java

@@ -69,7 +70,9 @@ public Expr apply(final List<Expr> args)
        lookupExtractorFactoryContainerProvider,
        lookupName,
        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

...main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java

+    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

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

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


nit: can use NullHandling.defaultStringValue()

Co-authored-by: Clint Wylie <[email protected]>

…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 <[email protected]> * Update docs/querying/sql-functions.md Co-authored-by: Clint Wylie <[email protected]> * Addressing comments --------- Co-authored-by: Clint Wylie <[email protected]>

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 <[email protected]>

Update docs/querying/sql-functions.md

51aa863

Co-authored-by: Clint Wylie <[email protected]>

Addressing comments

03f0ff5

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposing optional replaceMissingValueWith in lookup function and macros #14956

Exposing optional replaceMissingValueWith in lookup function and macros #14956

pranavbhole commented Sep 9, 2023 •

edited

Loading

soumyava Sep 9, 2023

pranavbhole Sep 10, 2023

soumyava Sep 9, 2023

pranavbhole Sep 10, 2023

clintropolis Sep 12, 2023

pranavbhole Sep 12, 2023

clintropolis Sep 12, 2023

pranavbhole Sep 12, 2023

clintropolis Sep 12, 2023

pranavbhole Sep 12, 2023

soumyava left a comment

clintropolis Sep 12, 2023

pranavbhole Oct 1, 2023

clintropolis Sep 12, 2023

pranavbhole Oct 1, 2023

clintropolis Sep 12, 2023

pranavbhole Oct 1, 2023

clintropolis Sep 12, 2023 •

edited

Loading

pranavbhole Oct 1, 2023

gianm Sep 14, 2023

pranavbhole Oct 1, 2023

gianm Sep 14, 2023

pranavbhole Oct 1, 2023

gianm Sep 14, 2023

pranavbhole Oct 1, 2023

clintropolis Oct 2, 2023

pranavbhole Oct 2, 2023

clintropolis Oct 2, 2023

pranavbhole Oct 2, 2023

clintropolis Oct 2, 2023

pranavbhole Oct 2, 2023

clintropolis Oct 2, 2023

pranavbhole Oct 2, 2023

Exposing optional replaceMissingValueWith in lookup function and macros #14956

Exposing optional replaceMissingValueWith in lookup function and macros #14956

Conversation

pranavbhole commented Sep 9, 2023 • edited Loading

Description

Release note

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

Choose a reason for hiding this comment

soumyava 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

clintropolis Sep 12, 2023 • 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

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

pranavbhole commented Sep 9, 2023 •

edited

Loading

clintropolis Sep 12, 2023 •

edited

Loading