Add FILLNULL command in PPL (#3032) #3075

normanj-bitquill · 2024-10-16T19:55:17Z

Description

Adds the FILLNULL command for PPL. FILLNULL will replace NULL values in specified fields.

Related Issues

Resolves #3032
Based on this PR for Spark: opensearch-project/opensearch-spark#723

Check List

[Y] New functionality includes testing.
[Y] New functionality has been documented.
[Y] New functionality has javadoc added.
[Y] New functionality has a user manual doc added.
[N/A] API changes companion pull request created.
[Y] Commits are signed per the DCO using --signoff.
[?] Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

An example query using fillnull.

    os> source=accounts | fields email, employer | fillnull with '<not found>' in email ;
    fetched rows / total rows = 4/4
    +-----------------------+----------+
    | email                 | employer |
    |-----------------------+----------|
    | [email protected]  | Pyrami   |
    | [email protected] | Netagy   |
    | <not found>           | Quility  |
    | [email protected]   | null     |
    +-----------------------+----------+

MaxKsyunz

Thank you for the PR! Could you add some integration tests? They live here.

jduo

I think we should add an ExplainTest to validate that you get an Eval physical plan when using FILLNULL.

normanj-bitquill · 2024-10-21T16:16:11Z

@MaxKsyunz I have added some integration tests.

normanj-bitquill · 2024-10-21T16:16:27Z

@jduo I have added an explain test.

currantw · 2024-10-24T16:29:29Z

docs/user/ppl/cmd/fillnull.rst

+    os> source=accounts | fields email, host | fillnull using email = '<not found>', host = '<no host>' ;
+    fetched rows / total rows = 4/4
+    +-----------------------+------------+
+    | email                 | host       |
+    |-----------------------+------------|
+    | [email protected]  | pyrami.com |
+    | [email protected] | netagy.com |
+    | <not found>           |            |
+    | [email protected]   | boink.com  |
+    +-----------------------+------------+


This is missing <no host> from the right column, right? Same for the table above, it looks like one <not found> is missing?

Suggested change

os> source=accounts | fields email, host | fillnull using email = '<not found>', host = '<no host>' ;

fetched rows / total rows = 4/4

+-----------------------+------------+

| email | host |

|-----------------------+------------|

| [email protected] | pyrami.com |

| [email protected] | netagy.com |

| <not found> | |

| [email protected] | boink.com |

+-----------------------+------------+

os> source=accounts | fields email, host | fillnull using email = '<not found>', host = '<no host>' ;

fetched rows / total rows = 4/4

+-----------------------+------------+

| email | host |

|-----------------------+------------|

| [email protected] | pyrami.com |

| [email protected] | netagy.com |

| <not found> | <no host> |

| [email protected] | boink.com |

+-----------------------+------------+

Updated the test.

In Example 1, it looks like <not found> is still missing from the host column..?

The test has been changed to use employer rather than host. null values are replaced with <no employer>.

Looks good to me. Thanks.

* Add FILLNULL command in PPL Signed-off-by: Norman Jordan <[email protected]>

Signed-off-by: Norman Jordan <[email protected]>

jduo · 2024-10-30T23:39:15Z

core/src/main/java/org/opensearch/sql/analysis/Analyzer.java

@@ -558,6 +559,29 @@ public LogicalPlan visitAD(AD node, AnalysisContext context) {
    return new LogicalAD(child, options);
  }

+  /** Build {@link LogicalAD} for fillnull command. */


This should link to LogicalEval instead of LogicalAD.

jduo · 2024-10-30T23:40:18Z

docs/user/ppl/cmd/fillnull.rst

@@ -0,0 +1,67 @@
+=============
+fillnull


Need to update category.json in doctest to get this to run with the documentation test.

Yury-Fridlyand · 2024-10-31T16:46:50Z

integ-test/src/test/java/org/opensearch/sql/ppl/FillNullCommandIT.java

+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | fields str2, num0 | fillnull with -1 in num0", TEST_INDEX_CALCS));


I'd show unchanged and filled column in the test (and in doctests and in PR description as well)

There is an integration test that updates one field, but not the other (testFillNullWithOtherField).

There is a test in the docs as well (the first query in fillnull.rst).

The description for this PR has been updated with a sample query.

Yury-Fridlyand · 2024-10-31T16:47:42Z

integ-test/src/test/resources/expectedOutput/ppl/explain_fillnull_push.json

+      }
+    ]
+  }
+}


Add eol please

Yury-Fridlyand · 2024-10-31T16:49:56Z

integ-test/src/test/java/org/opensearch/sql/ppl/FillNullCommandIT.java

+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | fields str2, num0 | fillnull using num0 = -1", TEST_INDEX_CALCS));


Please add more interesting tests there:

source=%s | fields str2, num0 | fillnull using num0 = num1 source=%s | fields str2, num0 | fillnull using num0 = cos(num1)

Added tests like this.

Yury-Fridlyand · 2024-10-31T16:51:49Z

docs/user/ppl/cmd/fillnull.rst

You need to list new file in table of content (see James's PR for example)

Yury-Fridlyand · 2024-10-31T16:52:53Z

core/src/main/java/org/opensearch/sql/ast/tree/FillNull.java

+
+@RequiredArgsConstructor
+@AllArgsConstructor
+public class FillNull extends UnresolvedPlan {


Do you mind adding a doc?

Added a Javadoc comment.

Signed-off-by: Norman Jordan <[email protected]>

MaxKsyunz reviewed Oct 18, 2024

View reviewed changes

YANG-DB previously approved these changes Oct 18, 2024

View reviewed changes

jduo suggested changes Oct 18, 2024

View reviewed changes

normanj-bitquill dismissed YANG-DB’s stale review via a1f584f October 21, 2024 16:15

currantw reviewed Oct 24, 2024

View reviewed changes

normanj-bitquill added 2 commits October 30, 2024 14:30

Add FILLNULL command in PPL (opensearch-project#3032)

7186378

* Add FILLNULL command in PPL Signed-off-by: Norman Jordan <[email protected]>

Added some integration tests for fillnull

33a04a6

Signed-off-by: Norman Jordan <[email protected]>

normanj-bitquill force-pushed the opensearch-3032 branch from a1f584f to 240d0d6 Compare October 30, 2024 22:47

Updated the multi field integration test

7d17615

Signed-off-by: Norman Jordan <[email protected]>

normanj-bitquill force-pushed the opensearch-3032 branch from 240d0d6 to 7d17615 Compare October 30, 2024 22:52

jduo suggested changes Oct 30, 2024

View reviewed changes

Yury-Fridlyand reviewed Oct 31, 2024

View reviewed changes

Added some more tests

a0f9c2c

Signed-off-by: Norman Jordan <[email protected]>

normanj-bitquill force-pushed the opensearch-3032 branch from d326da9 to a0f9c2c Compare November 8, 2024 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FILLNULL command in PPL (#3032) #3075

Add FILLNULL command in PPL (#3032) #3075

normanj-bitquill commented Oct 16, 2024 •

edited

Loading

MaxKsyunz left a comment •

edited

Loading

jduo left a comment

normanj-bitquill commented Oct 21, 2024

normanj-bitquill commented Oct 21, 2024

currantw Oct 24, 2024

normanj-bitquill Oct 30, 2024

currantw Oct 31, 2024

normanj-bitquill Nov 4, 2024

currantw Nov 4, 2024

jduo Oct 30, 2024

normanj-bitquill Oct 31, 2024

jduo Oct 30, 2024

normanj-bitquill Oct 31, 2024

Yury-Fridlyand Oct 31, 2024

normanj-bitquill Oct 31, 2024

Yury-Fridlyand Oct 31, 2024

normanj-bitquill Oct 31, 2024

Yury-Fridlyand Oct 31, 2024

normanj-bitquill Oct 31, 2024

Yury-Fridlyand Oct 31, 2024

normanj-bitquill Oct 31, 2024

Yury-Fridlyand Oct 31, 2024

normanj-bitquill Oct 31, 2024

Add FILLNULL command in PPL (#3032) #3075

Are you sure you want to change the base?

Add FILLNULL command in PPL (#3032) #3075

Conversation

normanj-bitquill commented Oct 16, 2024 • edited Loading

Description

Related Issues

Check List

MaxKsyunz left a comment • edited Loading

Choose a reason for hiding this comment

jduo left a comment

Choose a reason for hiding this comment

normanj-bitquill commented Oct 21, 2024

normanj-bitquill commented Oct 21, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanj-bitquill commented Oct 16, 2024 •

edited

Loading

MaxKsyunz left a comment •

edited

Loading