[SPARK-30612][SQL] Resolve qualified column name with v2 tables #27391

imback82 · 2020-01-30T03:04:23Z

What changes were proposed in this pull request?

This PR fixes the issue where queries with qualified columns like SELECT t.a FROM t would fail to resolve for v2 tables.

This PR would allow qualified column names in query as following:

SELECT testcat.ns1.ns2.tbl.foo FROM testcat.ns1.ns2.tbl
SELECT ns1.ns2.tbl.foo FROM testcat.ns1.ns2.tbl
SELECT ns2.tbl.foo FROM testcat.ns1.ns2.tbl
SELECT tbl.foo FROM testcat.ns1.ns2.tbl

Why are the changes needed?

This is a bug because you cannot qualify column names in queries.

Does this PR introduce any user-facing change?

Yes, now users can qualify column names for v2 tables.

How was this patch tested?

Added new tests.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala

SparkQA · 2020-01-30T03:13:24Z

Test build #117534 has finished for PR 27391 at commit dd59446.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AliasIdentifier(name: String, namespace: Seq[String])

imback82 · 2020-01-30T04:27:19Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+      checkAnswer(sql("select t.i from spark_catalog.default.t"), Row(1))
+      checkAnswer(sql("select default.t.i from spark_catalog.default.t"), Row(1))
+
+      // catalog name cannot be used for v1 tables.


Maybe we should allow using catalog name even for v1 tables since we allow it for table name resolution? (It should be a simple change.)

Makes sense. We can do it with another PR.

Using catalog name for v1 tables requires changes on the existing resolution rule (matchWithTwoOrLessQualifierParts) unless we fall back to new rule. This will be a 3.1 feature and I can update matchWithTwoOrLessQualifierParts now? I wanted to make sure before I get started. Thanks!

I'm not sure how you are going to implement it. Maybe we can discuss in your PR and decide if it should be 3.1 only or not.

OK, sounds good.

SparkQA · 2020-01-30T04:53:29Z

Test build #117539 has finished for PR 27391 at commit e744f81.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-30T08:05:02Z

Test build #117541 has finished for PR 27391 at commit 631304a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-30T08:05:02Z

Test build #117536 has finished for PR 27391 at commit 713c0fb.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2020-01-30T08:30:02Z

retest this please

SparkQA · 2020-01-30T13:11:35Z

Test build #117559 has finished for PR 27391 at commit 631304a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2020-01-30T19:49:07Z

@cloud-fan / @brkyvz This is now ready for review. Thanks!

brkyvz

I think you're doing the replacement in the wrong level. It's being pushed down far too much, requiring certain unnecessary changes. We actually have a great place to introduce the SubqueryAlias, and that's at ResolveTables in the Analyzer. Any UnresolvedRelation or UnresolvedV2Relation you get can be wrapped in a SubqueryAlias once resolved, and the great part is you have all the qualifiers you need to accomplish this right there.

I'm also a bit uneasy performing such drastic changes in expression resolution this late in the game. Can we leave the existing code and simply perform an additive change (we can follow up and clean things up after Spark 3.0)

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/identifiers.scala

sql/core/src/test/resources/sql-tests/results/group-by-filter.sql.out

brkyvz · 2020-01-31T00:13:52Z

I don't think so for ALTER TABLE. You don't have any ambiguity there with respect to columns, therefore wouldn't need to add qualifiers to the columns. I don't think v1 commands work with qualifiers either but we should check.

…

On Thu, Jan 30, 2020, 2:15 PM Terry Kim ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala <#27391 (comment)>: > @@ -389,7 +389,7 @@ case class DropTable( case class AlterTable( catalog: TableCatalog, ident: Identifier, - table: NamedRelation, + table: LogicalPlan, Don't we need to wrap this with SubqueryAlias (which is not a NamedRelation)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27391?email_source=notifications&email_token=ABIAE6Z7TODNEZUJ2PH5QHDRANGO3A5CNFSM4KNPCTF2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTXJLFI#discussion_r373222911>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIAE6YXO43QX23P5SHNKNTRANGO3ANCNFSM4KNPCTFQ> .

SparkQA · 2020-01-31T00:24:58Z

Test build #117583 has finished for PR 27391 at commit c414d7b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2020-01-31T05:37:42Z

You are right and I checked that v1 commands do not work with qualifiers. Thanks!

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

cloud-fan · 2020-01-31T12:37:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

     * Performs the lookup of DataSourceV2 Tables from v2 catalog.
     */
-    private def lookupV2Relation(identifier: Seq[String]): Option[DataSourceV2Relation] =
+    private def lookupV2Relation(identifier: Seq[String]): Option[LogicalPlan] =


The return type can still be DataSourceV2Relation?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/AttributeResolutionSuite.scala

cloud-fan · 2020-01-31T13:21:49Z

Agree with @brkyvz that we mostly only care about column qualifiers in SELECT, not DDL/DML commands. I think we only need to add SubqueryAlias when resolving UnresolvedRelation to v2 relations, to be consistent with v1 tables. An exception is UPDATE/DELETE/MERGE, where the conditions may contain qualified column names.

In general, this PR extends #17185 to support n-part qualifier. The change here makes sense to me.

imback82 · 2020-02-03T21:27:06Z

retest this please

SparkQA · 2020-02-04T01:01:16Z

Test build #117787 has finished for PR 27391 at commit 15c7003.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2020-02-04T01:05:33Z

retest this please

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/identifiers.scala

cloud-fan · 2020-02-04T04:46:20Z

LGTM except a few minor comments. while the major one is:https://github.com/apache/spark/pull/27391/files#r374470850

Thanks for fixing it!

SparkQA · 2020-02-04T05:25:52Z

Test build #117792 has finished for PR 27391 at commit 15c7003.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

brkyvz

LGTM. I'd love to see one more test case, but that can be added in a follow up. Thanks @imback82

brkyvz · 2020-02-04T23:52:58Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/AttributeResolutionSuite.scala

+class AttributeResolutionSuite extends SparkFunSuite {
+  val resolver = caseInsensitiveResolution
+
+  test("basic attribute resolution with namespaces") {


Can you add a test please where the table name and the column name is the same and make sure resolution works. Something like:

val attrs = Seq(AttributeReference("t", IntegerType)(qualifier = Seq("ns1", "ns2", "t"))) attrs.resolve(Seq("ns1", "ns2", "t"), resolver) match { case Some(attr) => fail() case _ => fail() }

SparkQA · 2020-02-05T02:33:10Z

Test build #117865 has finished for PR 27391 at commit 24daf5f.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-05T03:45:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala

+      attrs.forall(_.qualifier.length <= 2)
+
+    /** Match attributes for the case where all qualifiers in `attrs` have 2 or less parts. */
+    private def matchWithTwoOrLessPartQualifiers(


nit: matchWithTwoOrLessQualifierParts

cloud-fan · 2020-02-05T03:45:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala

+    /**
+     * Match attributes for the case where at least one qualifier in `attrs` has more than 2 parts.
+     */
+    private def matchWithThreeOrMorePartQualifiers(


cloud-fan · 2020-02-05T03:50:38Z

LGTM, let's add the test and get this merged!

SparkQA · 2020-02-05T07:56:20Z

Test build #117892 has finished for PR 27391 at commit c40895f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-05T07:59:11Z

retest this please

SparkQA · 2020-02-05T08:15:38Z

Test build #117910 has finished for PR 27391 at commit c40895f.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-05T10:46:37Z

retest this please

SparkQA · 2020-02-05T16:14:52Z

Test build #117927 has finished for PR 27391 at commit c40895f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-06T05:54:38Z

thanks, merging to master/3.0!

### What changes were proposed in this pull request? This PR fixes the issue where queries with qualified columns like `SELECT t.a FROM t` would fail to resolve for v2 tables. This PR would allow qualified column names in query as following: ```SQL SELECT testcat.ns1.ns2.tbl.foo FROM testcat.ns1.ns2.tbl SELECT ns1.ns2.tbl.foo FROM testcat.ns1.ns2.tbl SELECT ns2.tbl.foo FROM testcat.ns1.ns2.tbl SELECT tbl.foo FROM testcat.ns1.ns2.tbl ``` ### Why are the changes needed? This is a bug because you cannot qualify column names in queries. ### Does this PR introduce any user-facing change? Yes, now users can qualify column names for v2 tables. ### How was this patch tested? Added new tests. Closes #27391 from imback82/qualified_col. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit c27a616) Signed-off-by: Wenchen Fan <[email protected]>

brkyvz · 2020-02-06T17:48:20Z

Thanks @imback82 and @cloud-fan!

imback82 · 2020-02-06T18:04:45Z

Thanks @brkyvz and @cloud-fan for the review!

initial commit

dd59446

imback82 commented Jan 30, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala Show resolved Hide resolved

imback82 added 2 commits January 29, 2020 19:31

fix compilation error in hive test

713c0fb

catalog name support + add tests

e744f81

imback82 commented Jan 30, 2020

View reviewed changes

test fix

631304a

Added unit tests

c414d7b

imback82 changed the title ~~[WIP][SPARK-30612][SQL] Resolve qualified column name with v2 tables~~ [SPARK-30612][SQL] Resolve qualified column name with v2 tables Jan 30, 2020

brkyvz suggested changes Jan 30, 2020

View reviewed changes

cloud-fan reviewed Jan 31, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 31, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala Show resolved Hide resolved

cloud-fan reviewed Jan 31, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 31, 2020

View reviewed changes

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/AttributeResolutionSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 31, 2020

View reviewed changes

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/AttributeResolutionSuite.scala Outdated Show resolved Hide resolved

imback82 added 3 commits January 31, 2020 10:39

Address PR comments

a507120

Merge branch 'master' into qualified_col

709302b

refinement to minimize diff

6fb5799

cloud-fan reviewed Feb 4, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Feb 4, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/identifiers.scala Outdated Show resolved Hide resolved

address PR comments

24daf5f

brkyvz approved these changes Feb 4, 2020

View reviewed changes

cloud-fan reviewed Feb 5, 2020

View reviewed changes

address PR comments

c40895f

dongjoon-hyun added the SQL label Feb 5, 2020

cloud-fan closed this in c27a616 Feb 6, 2020

imback82 mentioned this pull request Feb 26, 2020

[SPARK-30782][SQL] Column resolution doesn't respect current catalog/namespace for v2 tables. #27532

Closed

[SPARK-30612][SQL] Resolve qualified column name with v2 tables #27391

[SPARK-30612][SQL] Resolve qualified column name with v2 tables #27391

Uh oh!

Conversation

imback82 commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

SparkQA commented Jan 30, 2020

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!

SparkQA commented Jan 30, 2020

Uh oh!

SparkQA commented Jan 30, 2020

Uh oh!

SparkQA commented Jan 30, 2020

Uh oh!

imback82 commented Jan 30, 2020

Uh oh!

SparkQA commented Jan 30, 2020

Uh oh!

imback82 commented Jan 30, 2020

Uh oh!

brkyvz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brkyvz commented Jan 31, 2020 via email

Uh oh!

SparkQA commented Jan 31, 2020

Uh oh!

imback82 commented Jan 31, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imback82 commented Feb 3, 2020

Uh oh!

SparkQA commented Feb 4, 2020

Uh oh!

imback82 commented Feb 4, 2020

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Feb 4, 2020

Uh oh!

SparkQA commented Feb 4, 2020

Uh oh!

brkyvz left a comment • 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!

imback82 commented Jan 30, 2020 •

edited

Loading

cloud-fan commented Jan 31, 2020 •

edited

Loading

brkyvz left a comment •

edited

Loading