Skip to content

Conversation

@peter-toth
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds some more WITH test cases as a follow-up to #24842

How was this patch tested?

Add new UTs.

@HyukjinKwon
Copy link
Member

ok to test

@HyukjinKwon
Copy link
Member

add to whitelist

@peter-toth
Copy link
Contributor Author

cc. @gatorsmile, @dongjoon-hyun

@SparkQA
Copy link

SparkQA commented Jun 24, 2019

Test build #106829 has finished for PR 24949 at commit efc96bb.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jun 24, 2019

Choose a reason for hiding this comment

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

According to @gatorsmile 's comment, could you add a t(x, x) test case, too?

Duplicate names within a single CTE definition are not allowed.

Copy link
Member

Choose a reason for hiding this comment

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

We need to capture the duplicate names earlier and issue a better error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I added the t(x, x) case, but it is allowed, and I think it is right to be allowed.
I thought @gatorsmile referred to the

WITH
  t(x) AS (SELECT 1),
  t(x) AS (SELECT 2)
SELECT * FROM t;

case, which is not.

@gatorsmile we capture the duplicate CTE name during parsing, I fixed the error message though.

If I got something wrong please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gatorsmile, @dongjoon-hyun do you think the PR is ok now?

@SparkQA
Copy link

SparkQA commented Jun 24, 2019

Test build #106845 has finished for PR 24949 at commit 815bb27.

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2019

Test build #106871 has finished for PR 24949 at commit 8fbae02.

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

@peter-toth
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Jun 25, 2019

Test build #106875 has finished for PR 24949 at commit 8fbae02.

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

@peter-toth
Copy link
Contributor Author

@gatorsmile, @dongjoon-hyun is there anything I can add to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can have a more meaningful message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this standard message comes from antlr when the query doesn't fit to the .g4 definition. I'm not sure if it can be fixed.

@peter-toth peter-toth changed the title [SPARK-28002][SQL][FOLLOWUP] Add more WITH test cases [SPARK-28002][SQL][FOLLOWUP] Fix duplicate CTE error message and add more WITH test cases Jul 5, 2019
@peter-toth peter-toth changed the title [SPARK-28002][SQL][FOLLOWUP] Fix duplicate CTE error message and add more WITH test cases [SPARK-28002][SQL][FOLLOWUP] Fix duplicate CTE error message and add more test cases Jul 5, 2019
@peter-toth peter-toth force-pushed the SPARK-28002-follow-up branch from 8fbae02 to 0c39fb2 Compare July 5, 2019 10:45
@SparkQA
Copy link

SparkQA commented Jul 5, 2019

Test build #107277 has finished for PR 24949 at commit 0c39fb2.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Merged to master.
Thank you, @peter-toth , @gatorsmile , @HyukjinKwon .

@peter-toth
Copy link
Contributor Author

Thanks for the review @dongjoon-hyun, @HyukjinKwon, @gatorsmile!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants