Skip to content

[SPARK-29030][SQL] Simplify lookupV2Relation#25735

Closed
jzhuge wants to merge 1 commit intoapache:masterfrom
jzhuge:lookupv2relation
Closed

[SPARK-29030][SQL] Simplify lookupV2Relation#25735
jzhuge wants to merge 1 commit intoapache:masterfrom
jzhuge:lookupv2relation

Conversation

@jzhuge
Copy link
Member

@jzhuge jzhuge commented Sep 9, 2019

What changes were proposed in this pull request?

Simplify the return type for lookupV2Relation which makes the 3 callers more straightforward.

How was this patch tested?

Existing unit tests.

@SparkQA
Copy link

SparkQA commented Sep 9, 2019

Test build #110364 has finished for PR 25735 at commit 855a8cf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • implicit class MultipartIdentifierHelper(parts: Seq[String])

@dongjoon-hyun
Copy link
Member

Hi, @jzhuge . Could you file a JIRA issue for this?

@jzhuge
Copy link
Member Author

jzhuge commented Sep 10, 2019

@dongjoon-hyun @HyukjinKwon Sure thing. Thanks for acting so quickly :)

@jzhuge jzhuge changed the title Simplify lookupV2Relation [SPARK-29030][SQL] Simplify lookupV2Relation Sep 10, 2019
@jzhuge
Copy link
Member Author

jzhuge commented Sep 10, 2019

Hi @brkyvz, do the changes look ok to you?

@SparkQA
Copy link

SparkQA commented Sep 10, 2019

Test build #110382 has finished for PR 25735 at commit a96e390.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • implicit class MultipartIdentifierHelper(parts: Seq[String])

changes))
case (Some(table), None) =>
Some(AlterTable(
sessionCatalog.asTableCatalog, // table being resolved means this exists
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use catalogManager.v2SessionCatalog to be consistent with lookupV2Relation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. This may inspire an even further simplification. Let me explore and report back.

@brkyvz
Copy link
Contributor

brkyvz commented Sep 11, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Sep 12, 2019

Test build #110488 has finished for PR 25735 at commit a96e390.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • implicit class MultipartIdentifierHelper(parts: Seq[String])

@SparkQA
Copy link

SparkQA commented Sep 12, 2019

Test build #110544 has finished for PR 25735 at commit f92a258.

  • This patch fails Scala style tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@jzhuge
Copy link
Member Author

jzhuge commented Sep 12, 2019

rebasing

@SparkQA
Copy link

SparkQA commented Sep 13, 2019

Test build #110546 has finished for PR 25735 at commit 58037d2.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • implicit class MultipartIdentifierHelper(parts: Seq[String])

@jzhuge
Copy link
Member Author

jzhuge commented Sep 13, 2019

Looking at the test failures

@SparkQA
Copy link

SparkQA commented Sep 13, 2019

Test build #110564 has finished for PR 25735 at commit a276e06.

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

@jzhuge
Copy link
Member Author

jzhuge commented Sep 13, 2019

retest please

@brkyvz
Copy link
Contributor

brkyvz commented Sep 17, 2019

@jzhuge Can you please rebase this?

@jzhuge
Copy link
Member Author

jzhuge commented Sep 17, 2019 via email

@SparkQA
Copy link

SparkQA commented Sep 18, 2019

Test build #110846 has finished for PR 25735 at commit eb14b7f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • implicit class MultipartIdentifierHelper(parts: Seq[String])

@brkyvz
Copy link
Contributor

brkyvz commented Sep 18, 2019

This LGTM. Merging to master. Thanks @jzhuge !

@asfgit asfgit closed this in ee94b5d Sep 18, 2019
@jzhuge
Copy link
Member Author

jzhuge commented Sep 18, 2019

Thanks @brkyvz !

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.

4 participants