[SPARK-19602][SQL][TESTS] Add tests for qualified column names #17067

skambha · 2017-02-25T23:55:10Z

What changes were proposed in this pull request?

Add tests covering different scenarios with qualified column names
Please see Section 2 in the design doc for the various test scenarios here
As part of SPARK-19602, changes are made to support three part column name. In order to aid in the review and to reduce the diff, the test scenarios are separated out into this PR.

How was this patch tested?

This is a test only change. The individual test suites were run successfully.

gatorsmile · 2017-02-26T06:33:45Z

ok to test

gatorsmile · 2017-02-26T06:36:43Z

Some test cases are negative; some test cases are expected to be supported. Could you split them from the positive test cases? Now some test cases are pretty large.

SparkQA · 2017-02-26T06:37:36Z

Test build #73479 has started for PR 17067 at commit 2f9937e.

gatorsmile · 2017-02-26T06:39:37Z

Uh. Forgot one more point.

Could you move these test cases to our new end-to-end SQL query suite into SQLQueryTestSuite ?

gatorsmile · 2017-02-26T06:40:38Z

Thank you for working on this! I like the coverage of qualified column names.

gatorsmile · 2017-02-26T17:12:12Z

retest this please

SparkQA · 2017-02-26T19:18:38Z

Test build #73494 has finished for PR 17067 at commit 2f9937e.

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

gatorsmile · 2017-02-28T04:27:11Z

We can move the remaining test cases to SQLQueryTestSuite , right? No need to put all of them in the same file.

skambha · 2017-02-28T05:04:41Z

Thanks much Xiao for the review and comments.

I have made the following changes:

Separated out the -ve cases from the +ve cases.
Moved positive tests and also the cases that should be supported into the SQLQueryTestSuite framework. A new test file columnresolution.sql and the corresponding master out file is added.
Clean up the ColumnResolutionSuite to remove cases that are covered in the SQLQueryTestSuite
I have kept the -ve cases in the ColumnResolutionSuite because the exprId shows up in the exception.
I also wanted to cover a case against a hive serde table so I have kept those tests in the ColumnResolutionSuite

Please advise if we should move any others. Thanks.

gatorsmile · 2017-02-28T05:10:09Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

How about these test cases for temporary views?

-- Test data. CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES (1, 1), (1, 2), (2, 1), (2, 2), (3, 1), (3, 2), (null, 1), (3, null), (null, null) AS testData(a, b);

Sure, let me look at converting these too. Thanks.

Also doable for global temporary view, I think

Hi Xiao, I have moved my new local temp view tests and the global temp view tests to the SQLQueryTestSuite framework as well. Please take a look. Thanks.

gatorsmile · 2017-02-28T05:17:37Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/ColumnResolutionSuite.scala

For the test cases you want to keep here, you can move it to sql/core. Why we need to test hive serde tables? Compared with data source tables, it is touching different code paths to resolve columns?

The logic to resolve the column in the LogicalPlan is same - there is no change there. I wanted to test the hive table to make sure that the qualifier information is correctly set. We update the qualifier info in MetastoreRelation so wanted to have coverage for hive table.

gatorsmile · 2017-02-28T05:18:18Z

sql/core/src/test/resources/sql-tests/inputs/columnresolution.sql

Please use upper case for SQL keywords.

SparkQA · 2017-02-28T05:43:49Z

Test build #73561 has finished for PR 17067 at commit e4f347e.

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

SparkQA · 2017-03-01T02:17:13Z

Test build #73622 has finished for PR 17067 at commit 1ae20ec.

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

…e out the -ve cases from +ve

…n change in exception

skambha · 2017-03-02T00:33:07Z

Changes to the SQLQueryTestSuite framework to mask the exprId so I can add the -ve cases as well using this framework.
Added -ve test cases to the SQLQueryTestSuite framework and so removed the hive specific test suite. For the hive table testcase, I will add that test as part of the actual code changes PR.
I synced up the codeline and there was one test output inner-join.sql.out that needed a comment to be updated, so I have updated that as well.

SparkQA · 2017-03-02T02:24:46Z

Test build #73722 has finished for PR 17067 at commit 5594eb0.

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

gatorsmile · 2017-03-02T04:26:26Z

sql/core/src/test/resources/sql-tests/inputs/columnresolution-negative.sql

+-- reset
+set spark.sql.crossJoin.enabled = false;
+DROP DATABASE mydb1 CASCADE;
+DROP DATABASE mydb2 CASCADE;


Please add one empty space after this line

gatorsmile · 2017-03-02T04:26:57Z

sql/core/src/test/resources/sql-tests/inputs/columnresolution-negative.sql

+SELECT mydb1.t1.i1 FROM t1;
+
+-- reset
+set spark.sql.crossJoin.enabled = false;


Move this to line 24

gatorsmile · 2017-03-02T04:27:49Z

sql/core/src/test/resources/sql-tests/inputs/columnresolution-views.sql

@@ -0,0 +1,25 @@
+-- Tests for qualified column names for the view code-path
+-- Test scenario with Temporary view
+CREATE OR REPLACE TEMPORARY VIEW table1 AS SELECT 2 AS i1;


table1 -> view1

gatorsmile · 2017-03-02T04:28:17Z

sql/core/src/test/resources/sql-tests/inputs/columnresolution-views.sql

+DROP VIEW table1;
+
+-- Test scenario with Global Temp view
+CREATE OR REPLACE GLOBAL TEMPORARY VIEW t1 as SELECT 1 as i1;


t1 -> view1

gatorsmile · 2017-03-02T04:30:55Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+        val msg = if (a.plan.nonEmpty) a.getSimpleMessage else a.getMessage
        (StructType(Seq.empty),
-          Seq(a.getClass.getName, a.getSimpleMessage.replaceAll("#\\d+", "#x")))
+          Seq(a.getClass.getName, msg.replaceAll("#\\d+", "#x")))


Nit: (StructType(Seq.empty), Seq(a.getClass.getName, msg.replaceAll("#\\d+", "#x")))

gatorsmile · 2017-03-02T04:32:14Z

sql/core/src/test/resources/sql-tests/results/columnresolution-negative.sql.out

+struct<>
+-- !query 9 output
+org.apache.spark.sql.AnalysisException
+Reference 't1.i1' is ambiguous, could be: i1#x, i1#x.; line 1 pos 7


To the other reviewer, this is the reason why we need to make a change in SQLQueryTestSuite.scala

gatorsmile · 2017-03-02T04:33:51Z

sql/core/src/test/resources/sql-tests/inputs/columnresolution.sql

+INSERT INTO t5 VALUES(1, (2, 3));
+SELECT t5.i1 FROM t5;
+SELECT t5.t5.i1 FROM t5;
+SELECT t5.t5.i1 FROM mydb1.t5;


Add two more cases for verifying *

gatorsmile · 2017-03-02T04:34:44Z

Generally, it looks good to me.

SparkQA · 2017-03-02T19:02:14Z

Test build #73779 has finished for PR 17067 at commit b2e411c.

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

gatorsmile · 2017-03-03T05:18:27Z

LGTM

gatorsmile · 2017-03-03T05:20:13Z

Thanks! Merging to master.

skambha · 2017-03-03T06:21:19Z

Thanks a lot Xiao.

…n name ( 3 part name) ## What changes were proposed in this pull request? The design details is attached to the JIRA issue [here](https://drive.google.com/file/d/1zKm3aNZ3DpsqIuoMvRsf0kkDkXsAasxH/view) High level overview of the changes are: - Enhance the qualifier to be more than one string - Add support to store the qualifier. Enhance the lookupRelation to keep the qualifier appropriately. - Enhance the table matching column resolution algorithm to account for qualifier being more than a string. - Enhance the table matching algorithm in UnresolvedStar.expand - Ensure that we continue to support select t1.i1 from db1.t1 ## How was this patch tested? - New tests are added. - Several test scenarios were added in a separate [test pr 17067](#17067). The tests that were not supported earlier are marked with TODO markers and those are now supported with the code changes here. - Existing unit tests ( hive, catalyst and sql) were run successfully. Closes #17185 from skambha/colResolution. Authored-by: Sunitha Kambhampati <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

gatorsmile reviewed Feb 28, 2017

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/columnresolution.sql Outdated

Copy link

Member

gatorsmile Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use upper case for SQL keywords.

skambha force-pushed the colResolutionTests branch from e4f347e to 1ae20ec Compare March 1, 2017 00:02

skambha added 15 commits March 1, 2017 15:42

Add tests to cover scenarios for the column resolution

5257983

Add tests for global temp views

685a53d

Add test with local view

596b3e8

newline in end

cdd1468

uppercase struct

e95c33d

fix style and use ctas

715bc04

Add column resolution tests to the SQLQueryTestSuite and also separat…

040e7dd

…e out the -ve cases from +ve

Remove the -ve cases from the SQLQueryTestSuite because the exprId ca…

feeb8a7

…n change in exception

Tests are moved to the SQLQueryTestSuite

129c0c4

Remove empty lines

4700f39

Remove unused import

158016f

Move view tests also to SQLQueryTestSuite

1c5cc4b

Rebase with master

14ae125

upper casing SQL

1accdcf

Add -ve tests to the SQLQueryTestSuite framework

102e555

skambha added 3 commits March 1, 2017 15:42

This file isnt updated properly in Spark-19766

f01c884

Change to SQLQueryTestSuite to mask the id

14fc6fd

Delete the ColumnResolutionSuite

5594eb0

skambha force-pushed the colResolutionTests branch from 1ae20ec to 5594eb0 Compare March 2, 2017 00:28

gatorsmile reviewed Mar 2, 2017

View reviewed changes

address comments

b2e411c

asfgit closed this in f37bb14 Mar 3, 2017

skambha mentioned this pull request Mar 7, 2017

[SPARK-19602][SQL] Support column resolution of fully qualified column name ( 3 part name) #17185

Closed

[SPARK-19602][SQL][TESTS] Add tests for qualified column names #17067

[SPARK-19602][SQL][TESTS] Add tests for qualified column names #17067

Uh oh!

Conversation

skambha commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile commented Feb 26, 2017

Uh oh!

gatorsmile commented Feb 26, 2017

Uh oh!

SparkQA commented Feb 26, 2017

Uh oh!

gatorsmile commented Feb 26, 2017

Uh oh!

gatorsmile commented Feb 26, 2017

Uh oh!

gatorsmile commented Feb 26, 2017

Uh oh!

SparkQA commented Feb 26, 2017

Uh oh!

gatorsmile commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skambha commented Feb 28, 2017

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

skambha commented Mar 2, 2017

Uh oh!

SparkQA commented Mar 2, 2017

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!

gatorsmile Mar 2, 2017 • 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!

gatorsmile commented Mar 2, 2017

Uh oh!

SparkQA commented Mar 2, 2017

Uh oh!

gatorsmile commented Mar 3, 2017

Uh oh!

gatorsmile commented Mar 3, 2017

Uh oh!

skambha commented Mar 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

skambha commented Feb 25, 2017 •

edited

Loading

gatorsmile commented Feb 28, 2017 •

edited

Loading

gatorsmile Mar 2, 2017 •

edited

Loading