Join side aliases should be optional #862

LantaoJin · 2024-11-01T09:01:32Z

Description

Imaging a case as following: Assume table1, table2, and table3 all contain a column id.

select
  *
from
  table1 t1,
  table2 t2,
  table3 t3
where
 t1.id = t2.id
 and t1.id = t3.id

To rewrite above SQL query to PPL query, we will get

source = table1
| join left = t1 right = t2 ON t1.id = t2.id table2
| join left = l1 right = t3 ON t1.id = t3.id table3 // <------ issue here!

The PPL query throws an exception with message:

t1.id cannot be resolved, Did you mean one of the following? [l1.id, l1.id, t3.id].

It because the required left side alias l1 overrides the table alias t1 and t2.

Its logical plan looks

'Project [*]
+- 'Filter (('t1.id = 't2.id) AND ('t1.id = 't3.id)) <------ t1.id cannot be resolved
   +- 'Join Inner
      :- 'SubqueryAlias l1  <------ issue root cause
      :  +- 'Join Inner
      :    :- 'SubqueryAlias t1
      :    :  +- 'UnresolvedRelation [table1], [], false
      :    +- 'SubqueryAlias t2
      :       +- 'UnresolvedRelation [table2], [], false
      +- 'SubqueryAlias t3
         +- 'UnresolvedRelation [table3], [], false

Check #857 for details

Related Issues

Resolves #857

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: Lantao Jin <ltjin@amazon.com>

LantaoJin · 2024-11-01T09:04:45Z

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

-  }
-
-  /** Optional alias name for the relation. */
-  @Setter @Getter private String alias;


The alias in Relation could be removed since we have the SubqueryAlias in AST: https://github.com/opensearch-project/opensearch-spark/blob/main/ppl-spark-integration/src/main/java/org/opensearch/sql/ast/tree/SubqueryAlias.java

Remaining this member causes potential issue if conflicts. All table with alias in PPL should add a SubqueryAlias such as #862 (comment)

@LantaoJin can u give an example for a union query ?
source = a as table_a, b as table_b, c as table_c | ...

@LantaoJin can u give an example for a union query ? source = a as table_a, b as table_b, c as table_c | ...

No, we didn't support this pattern, see

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

Line 323 in b183b4a

: tableSource (COMMA tableSource)* (AS alias = qualifiedName)?

But we supported the pattern:
source = table1, table2, table3 as t | where t.name = 'Molly'
The plan of above is

'Union true, true :- 'Project [*] : +- 'Filter ('t.name = Molly) : +- 'SubqueryAlias t : +- 'UnresolvedRelation [table1], [], false :- 'Project [*] : +- 'Filter ('t.name = Molly) : +- 'SubqueryAlias t : +- 'UnresolvedRelation [table2], [], false +- 'Project [*] +- 'Filter ('t.name = Molly) +- 'SubqueryAlias t +- 'UnresolvedRelation [table3], [], false

I will add an UT and IT of the above case in latest commit.

LantaoJin · 2024-11-01T09:08:12Z

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

-   *
-   * @return table name
-   */
-  public List<String> getTableName() {


Never used.

LantaoJin · 2024-11-01T09:11:07Z

...on/src/test/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanJoinTranslatorTestSuite.scala

+         | | inner JOIN left = l right = r ON l.id = r.id $testTable2
+         | | left JOIN left = l right = r ON l.name = r.name $testTable3
+         | | cross JOIN left = l right = r $testTable4


Comma is optional, I am trying to reduce this usage. But keep the comma won't fail query.

LantaoJin · 2024-11-01T09:11:11Z

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java

-        ? new Relation(ctx.tableSource().stream().map(this::internalVisitExpression).collect(Collectors.toList()))
-        : new Relation(ctx.tableSource().stream().map(this::internalVisitExpression).collect(Collectors.toList()), ctx.alias.getText());
+    Relation relation = new Relation(ctx.tableSource().stream().map(this::internalVisitExpression).collect(Collectors.toList()));
+    return ctx.alias != null ? new SubqueryAlias(ctx.alias.getText(), relation) : relation;


We add SubqueryAlias ast node instead of adding the alias string into Relation.

LantaoJin · 2024-11-01T09:11:17Z

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

-   *
-   * @return table name
-   */
-  public List<String> getTableName() {


Never used.

LantaoJin · 2024-11-01T10:03:54Z

Without this fixing, q21 in TPCH couldn't be rewritten to PPL. Link #830

YANG-DB · 2024-11-05T01:42:44Z

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

 @ToString
+@Getter
 @EqualsAndHashCode(callSuper = false)
 @RequiredArgsConstructor
 public class Relation extends UnresolvedPlan {
  private static final String COMMA = ",";

  private final List<UnresolvedExpression> tableName;


@LantaoJin lates change the name to indicate it may contain multi-part

YANG-DB · 2024-11-05T01:44:30Z

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

-  }
-
-  /** Optional alias name for the relation. */
-  @Setter @Getter private String alias;


@LantaoJin can u give an example for a union query ?
source = a as table_a, b as table_b, c as table_c | ...

YANG-DB · 2024-11-05T01:49:54Z

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/CatalystPlanContext.java

+    public Expression resolveJoinCondition(
+            UnresolvedExpression expr,
+            BiFunction<UnresolvedExpression, CatalystPlanContext, Expression> transformFunction) {
+        isResolvingJoinCondition = true;


can u make this an atomic bool ?

CatalystPlanContext is generated by each query. All members in CatalystPlanContext should be thread-safe. There is no multi-threads need to check and change the boolean and other structures of CatalystPlanContext.

lets add this note on the CatalystPlanContext class description for better clarity

YANG-DB · 2024-11-05T01:53:49Z

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java

    UnresolvedPlan rightRelation = visit(ctx.tableOrSubqueryClause());
-    UnresolvedPlan right = new SubqueryAlias(rightAlias, rightRelation);
+    UnresolvedPlan right;


please add some comment explaining the condition

Signed-off-by: Lantao Jin <ltjin@amazon.com>

YANG-DB · 2024-11-05T17:07:30Z

@LantaoJin also plz resolve conflicts...

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin · 2024-11-06T01:21:41Z

Conflicts is related to imports. CI tests succeed with the related files. Merging to main.

* Join side aliases should be optional Signed-off-by: Lantao Jin <ltjin@amazon.com> * address comments Signed-off-by: Lantao Jin <ltjin@amazon.com> * typo Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com>

Join side aliases should be optional

94a546c

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin commented Nov 1, 2024

View reviewed changes

LantaoJin marked this pull request as ready for review November 1, 2024 10:14

LantaoJin requested review from dai-chen, rupal-bq, mengweieric, vamsimanohar, penghuo, seankao-az, anirudha, kaituo, YANG-DB, noCharger and ykmr1224 as code owners November 1, 2024 10:14

LantaoJin added 0.6 Lang:PPL Pipe Processing Language support labels Nov 1, 2024

YANG-DB requested changes Nov 5, 2024

View reviewed changes

LantaoJin added 2 commits November 5, 2024 15:05

address comments

c47988c

Signed-off-by: Lantao Jin <ltjin@amazon.com>

typo

76c4e31

Signed-off-by: Lantao Jin <ltjin@amazon.com>

YANG-DB approved these changes Nov 5, 2024

View reviewed changes

Merge branch 'main' into pr/issues/857

ff5abaa

Signed-off-by: Lantao Jin <ltjin@amazon.com>

LantaoJin merged commit cfd41a3 into opensearch-project:main Nov 6, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Join side aliases should be optional #862

Join side aliases should be optional #862

LantaoJin commented Nov 1, 2024 •

edited

Loading

LantaoJin Nov 1, 2024 •

edited

Loading

YANG-DB Nov 5, 2024

LantaoJin Nov 5, 2024

LantaoJin Nov 5, 2024

LantaoJin Nov 1, 2024

LantaoJin Nov 1, 2024

LantaoJin Nov 1, 2024

LantaoJin Nov 1, 2024

LantaoJin commented Nov 1, 2024 •

edited

Loading

YANG-DB Nov 5, 2024

LantaoJin Nov 5, 2024

YANG-DB Nov 5, 2024

YANG-DB Nov 5, 2024

LantaoJin Nov 5, 2024 •

edited

Loading

YANG-DB Nov 5, 2024

YANG-DB Nov 5, 2024

LantaoJin Nov 5, 2024

YANG-DB commented Nov 5, 2024

LantaoJin commented Nov 6, 2024

Join side aliases should be optional #862

Join side aliases should be optional #862

Conversation

LantaoJin commented Nov 1, 2024 • edited Loading

Description

Related Issues

Check List

LantaoJin Nov 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LantaoJin commented Nov 1, 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

LantaoJin Nov 5, 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

YANG-DB commented Nov 5, 2024

LantaoJin commented Nov 6, 2024

LantaoJin commented Nov 1, 2024 •

edited

Loading

LantaoJin Nov 1, 2024 •

edited

Loading

LantaoJin commented Nov 1, 2024 •

edited

Loading

LantaoJin Nov 5, 2024 •

edited

Loading