Support flatten with alias #927

qianheng-aws · 2024-11-18T07:24:28Z

Description

Support flatten with alias. e.g.

source=table | flatten coor as (altitude, latitude, longitude)

source=table | flatten struct as subfield

Related Issues

Resolve #911

We can use alias to avoid duplicate column name in the final result.

Check List

Updated documentation (docs/ppl-lang/README.md)
Implemented unit tests
Implemented tests for combination with other commands
New added source code should include a copyright header
Commits are signed per the DCO using --signoff

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.

Signed-off-by: Heng Qian <[email protected]>

LantaoJin · 2024-11-18T07:56:54Z

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4

+   : qualifiedName (COMMA qualifiedName)* # identsAsQualifiedNameSeq
+   | LT_PRTHS qualifiedName (COMMA qualifiedName)* RT_PRTHS # identsAsQualifiedNameSeq


This grammar allows four patterns:

flatten struct_col2 as field1

flatten struct_col2 as (field1).

flatten struct_col2 as field1, field2

flatten struct_col2 as (field1, field2).

Is ( ) necessary or restrict to two?

flatten struct_col2 as field1

flatten struct_col2 as (field1, field2)

It's not restrict to two, as you said, both 4 patterns are all allowed. For most of the case, both is ok with or without (), but having () is safer to avoid ambiguous.

For example, in SQL, when having sql like select flatten(a) as b, c, field c will be parsed as a child of select instead of flatten alias, so put alias inner brace will avoid this. I'm not sure if PPL will have case like that.

For example, in SQL, when having sql like select flatten(a) as b, c, field c will be parsed as a child of select instead of flatten alias, so put alias inner brace will avoid this. I'm not sure if PPL will have case like that.

Yes. That's my point. How about only remains

flatten struct_col2 as field1

flatten struct_col2 as (field1)

flatten struct_col2 as (field1, field2)

or

flatten struct_col2 as (field1)

flatten struct_col2 as (field1, field2)

@YANG-DB any thoughts?

@LantaoJin I personally dont mind the extra verbose - so Im good with @qianheng-aws suggestion...

ppl-spark-integration/src/main/java/org/opensearch/sql/ast/tree/Flatten.java

LantaoJin · 2024-11-18T08:08:04Z

...-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLFlattenITSuite.scala

@@ -347,4 +347,30 @@ class FlintSparkPPLFlattenITSuite
    val expectedPlan = Project(Seq(UnresolvedStar(None)), flattenMultiValue)
    comparePlans(logicalPlan, expectedPlan, checkAnalysis = false)
  }
+
+  test("flatten struct nested table using alias") {


Case 1 will be similar to this test except that it aliased field2 to field2_2.

opensearch-spark/integ-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLFlattenITSuite.scala

Line 298 in b050da3

frame.columns.sameElements(Array("int_col", "field2", "subfield", "field2", "subfield")))

.

Case 1 will

I mean that will the duplicated field2_2 in query fail?

source = ... | flatten struct_col as (field1, field2_2) | flatten struct_col2 as (field1, field2_2)

LantaoJin · 2024-11-18T08:10:31Z

High level question: why the solution changed to add aliases instead of distinguishing by struct_col.field1 and struct_col2.field1? The second solution should be more graceful.

LantaoJin · 2024-11-18T08:21:14Z

source=table | flatten struct as subfield seems quite confused.
I'd like to change the PPL design to
source=table | flatten struct rename origin_subfield1 as new_subfield1, origin_subfield1 as new_subfield1,... even in the alias solution.

LantaoJin · 2024-11-18T08:26:12Z

@qianheng-aws Please keep this PR DRAFT before PPL design review is passed.

qianheng-aws · 2024-11-18T08:52:26Z

High level question: why the solution changed to add aliases instead of distinguishing by struct_col.field1 and struct_col2.field1? The second solution should be more graceful.

We cannot do that in parsing phase. That's say, we don't know the actual fields inner the struct field. So I give up that solution and choose to support alias syntax instead of giving alias automatically inner our parser, then users can still do that to avoid duplicate column name in their PPL.

And as said in the issue #911 (comment), it's actually a common issue for asyn-query not just for flatten. I think maybe support alias is a more appropriate way to address such issue.

qianheng-aws · 2024-11-18T09:00:46Z

source=table | flatten struct as subfield seems quite confused. I'd like to change the PPL design to source=table | flatten struct rename origin_subfield1 as new_subfield1, origin_subfield1 as new_subfield1,... even in the alias solution.

Then we need to figure out another way to pass that fields mapping to Spark operator Generate. Unfortunately, Spark doesn't have such mechanism, it only support make alias to the original operator output in sequence.

LantaoJin · 2024-11-18T09:09:25Z

source=table | flatten struct as subfield seems quite confused. I'd like to change the PPL design to source=table | flatten struct rename origin_subfield1 as new_subfield1, origin_subfield1 as new_subfield1,... even in the alias solution.

Then we need to figure out another way to pass that fields mapping to Spark operator Generate. Unfortunately, Spark doesn't have such mechanism, it only support make alias to the original operator output in sequence.

Ok, I see now. So for the column struct_col STRUCT<field1: STRUCT<subfield:STRING>, field2: INT> case, we must add aliases sequence such as | flatten struct_col as (field1_1, field2_2).

LantaoJin · 2024-11-18T09:15:05Z

docs/ppl-lang/ppl-flatten-command.md

@@ -7,9 +7,10 @@ Using `flatten` command to flatten a field of type:


 ### Syntax
-`flatten <field>`
+`flatten <field> [As alias]`


[as aliasSequence]

docs/ppl-lang/ppl-flatten-command.md

LantaoJin · 2024-11-18T09:18:53Z

...-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLFlattenITSuite.scala

@@ -347,4 +347,30 @@ class FlintSparkPPLFlattenITSuite
    val expectedPlan = Project(Seq(UnresolvedStar(None)), flattenMultiValue)
    comparePlans(logicalPlan, expectedPlan, checkAnalysis = false)
  }
+
+  test("flatten struct nested table using alias") {


Case 1 will

I mean that will the duplicated field2_2 in query fail?

source = ... | flatten struct_col as (field1, field2_2) | flatten struct_col2 as (field1, field2_2)

LantaoJin · 2024-11-18T09:21:35Z

...-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLFlattenITSuite.scala

+                       | source = $structNestedTable
+                       | | flatten struct_col
+                       | | flatten field1 as subfield_1
+                       | | flatten struct_col2 as (field1, field2_2)


Please add a test case: it should fail

source =... | flatten struct_col as (field1_1)

LantaoJin · 2024-11-18T09:24:35Z

The basic approach works for me. Only some minor comments. @YANG-DB please take a look.

Signed-off-by: Heng Qian <[email protected]>

YANG-DB

@qianheng-aws plz add additional flatten query sample here

YANG-DB · 2024-11-18T19:54:26Z

...t/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanFlattenCommandTranslatorTestSuite.scala

@@ -153,4 +153,45 @@ class PPLLogicalPlanFlattenCommandTranslatorTestSuite
    comparePlans(expectedPlan, logPlan, checkAnalysis = false)
  }

+  test("test flatten with one alias") {


@qianheng-aws plz add additional flatten with aliases queries unit tests

@YANG-DB we already have but named "test flatten with alias list"

Signed-off-by: Heng Qian <[email protected]>

LantaoJin · 2024-11-19T03:40:04Z

...-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLFlattenITSuite.scala

+
+    // Throw AnalysisException if The number of aliases supplied in the AS clause does not match the
+    // number of columns output
+    assertThrows[AnalysisException] {


Add error message assertion in case we could be informed any behaviour changes in Spark upgrading.

LantaoJin · 2024-11-19T03:42:43Z

...-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLFlattenITSuite.scala

+    // duplicate alias names
+    val frame2 = sql(s"""
+                       | source = $structNestedTable
+                       | | flatten struct_col as (field1, field2_2)
+                       | | flatten field1 as subfield_1
+                       | | flatten struct_col2 as (field1, field2_2)
+                       | | flatten field1 as subfield_2


duplicated alias case I mentioned is

| flatten struct_col as (field1, field2_2) | flatten struct_col2 as (field1, field2_2)

Any concern on your case? I think the purpose is to test on duplicate alias names between 2 flatten command. I only change the field name in the original test cases to satisfy our purpose, so we can reuse the constructed expected results.

LantaoJin · 2024-11-19T03:43:34Z

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4

+   : qualifiedName (COMMA qualifiedName)* # identsAsQualifiedNameSeq
+   | LT_PRTHS qualifiedName (COMMA qualifiedName)* RT_PRTHS # identsAsQualifiedNameSeq


@YANG-DB any thoughts?

YANG-DB

LGTM - thanks @qianheng-aws

Signed-off-by: Heng Qian <[email protected]>

LantaoJin · 2024-11-20T04:00:06Z

...-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLFlattenITSuite.scala

    // Compare the results
-    implicit val rowOrdering: Ordering[Row] = Ordering.by[Row, Int](_.getAs[Int](0))
-    assert(results.sorted.sameElements(expectedResults.sorted))
+    assert(results.sorted.sameElements(expectedResults))


try using assertSameRows in future which won't consider any ordering.

Sure, seems that function will do sorting internally

* Support flatten with alias Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Add flatten with alias example Signed-off-by: Heng Qian <[email protected]> * Add IT for ambiguous exception Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit dc690b4) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Support flatten with alias * Address comments * Address comments * Add flatten with alias example * Add IT for ambiguous exception --------- (cherry picked from commit dc690b4) Signed-off-by: Heng Qian <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Support flatten with alias Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Add flatten with alias example Signed-off-by: Heng Qian <[email protected]> * Add IT for ambiguous exception Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]>

Support flatten with alias

0899d53

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws requested review from dai-chen, mengweieric, vmmusings, penghuo, seankao-az, anirudha, kaituo, YANG-DB, noCharger, LantaoJin and ykmr1224 as code owners November 18, 2024 07:24

LantaoJin reviewed Nov 18, 2024

View reviewed changes

qianheng-aws marked this pull request as draft November 18, 2024 09:02

LantaoJin reviewed Nov 18, 2024

View reviewed changes

Address comments

9d9ad80

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws marked this pull request as ready for review November 18, 2024 09:51

Address comments

e494c5d

Signed-off-by: Heng Qian <[email protected]>

YANG-DB added Lang:PPL Pipe Processing Language support 0.6 sanity-0.6 backport 0.6 labels Nov 18, 2024

YANG-DB reviewed Nov 18, 2024

View reviewed changes

Add flatten with alias example

dd72b1d

Signed-off-by: Heng Qian <[email protected]>

LantaoJin reviewed Nov 19, 2024

View reviewed changes

YANG-DB approved these changes Nov 19, 2024

View reviewed changes

Add IT for ambiguous exception

b7cdf91

Signed-off-by: Heng Qian <[email protected]>

LantaoJin reviewed Nov 20, 2024

View reviewed changes

LantaoJin merged commit dc690b4 into opensearch-project:main Nov 20, 2024
4 checks passed

opensearch-trigger-bot bot mentioned this pull request Nov 20, 2024

[Backport 0.6] Support flatten with alias #935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support flatten with alias #927

Support flatten with alias #927

qianheng-aws commented Nov 18, 2024

LantaoJin Nov 18, 2024

qianheng-aws Nov 18, 2024

LantaoJin Nov 18, 2024 •

edited

Loading

LantaoJin Nov 19, 2024

YANG-DB Nov 19, 2024

LantaoJin Nov 18, 2024

qianheng-aws Nov 18, 2024

LantaoJin Nov 18, 2024

LantaoJin commented Nov 18, 2024 •

edited

Loading

LantaoJin commented Nov 18, 2024

LantaoJin commented Nov 18, 2024 •

edited

Loading

qianheng-aws commented Nov 18, 2024 •

edited

Loading

qianheng-aws commented Nov 18, 2024

LantaoJin commented Nov 18, 2024

LantaoJin Nov 18, 2024

LantaoJin Nov 18, 2024

LantaoJin Nov 18, 2024

LantaoJin commented Nov 18, 2024

YANG-DB left a comment

YANG-DB Nov 18, 2024 •

edited

Loading

qianheng-aws Nov 19, 2024 •

edited

Loading

LantaoJin Nov 19, 2024

LantaoJin Nov 19, 2024

qianheng-aws Nov 19, 2024

LantaoJin Nov 19, 2024

YANG-DB left a comment

LantaoJin Nov 20, 2024 •

edited

Loading

qianheng-aws Nov 20, 2024

		: qualifiedName (COMMA qualifiedName)* # identsAsQualifiedNameSeq
		\| LT_PRTHS qualifiedName (COMMA qualifiedName)* RT_PRTHS # identsAsQualifiedNameSeq

Support flatten with alias #927

Support flatten with alias #927

Conversation

qianheng-aws commented Nov 18, 2024

Description

Related Issues

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LantaoJin Nov 18, 2024 • 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

LantaoJin commented Nov 18, 2024 • edited Loading

LantaoJin commented Nov 18, 2024

LantaoJin commented Nov 18, 2024 • edited Loading

qianheng-aws commented Nov 18, 2024 • edited Loading

qianheng-aws commented Nov 18, 2024

LantaoJin commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LantaoJin commented Nov 18, 2024

YANG-DB left a comment

Choose a reason for hiding this comment

YANG-DB Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

qianheng-aws Nov 19, 2024 • 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

YANG-DB left a comment

Choose a reason for hiding this comment

LantaoJin Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LantaoJin Nov 18, 2024 •

edited

Loading

LantaoJin commented Nov 18, 2024 •

edited

Loading

LantaoJin commented Nov 18, 2024 •

edited

Loading

qianheng-aws commented Nov 18, 2024 •

edited

Loading

YANG-DB Nov 18, 2024 •

edited

Loading

qianheng-aws Nov 19, 2024 •

edited

Loading

LantaoJin Nov 20, 2024 •

edited

Loading