Skip to content

Conversation

@alexliu68
Copy link

The pull only fixes the parsing error and changes API to use tableIdentifier. Joining different catalog datasource related change is not done in this pull.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should probably just be a single rule. With something like: repsep(ident, ",") for the table identifier.

Copy link
Author

Choose a reason for hiding this comment

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

I change it to rep1sep(ident, ".")

@marmbrus
Copy link
Contributor

marmbrus commented Jan 9, 2015

Thanks for doing this cleanup! Overall I think this looks pretty good. I made a few small comments. One other very minor nit. It seems a little odd to me to be using IndexedSeq everywhere instead of just Seq or :: Nil.

@marmbrus
Copy link
Contributor

marmbrus commented Jan 9, 2015

Also, can you fix the PR title to avoid truncating.

@marmbrus
Copy link
Contributor

marmbrus commented Jan 9, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Jan 9, 2015

Test build #25340 has started for PR 3941 at commit 29e5e55.

  • This patch merges cleanly.

@alexliu68 alexliu68 changed the title [SPARK-4943][SQL] Allow table name having dot to support db/catalog ... [SPARK-4943][SQL] Allow table name having dot for db/catalog Jan 9, 2015
@alexliu68
Copy link
Author

I use IndexedSeq instead Seq for IndexedSeq is faster to access element by lift. I can revert them back to Seq.

@marmbrus
Copy link
Contributor

marmbrus commented Jan 9, 2015

Its minor, but it seems unlikely that it it noticeably faster in this instance.

@SparkQA
Copy link

SparkQA commented Jan 9, 2015

Test build #25340 has finished for PR 3941 at commit 29e5e55.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25340/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 10, 2015

Test build #25356 has started for PR 3941 at commit 343ae27.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 10, 2015

Test build #25356 has finished for PR 3941 at commit 343ae27.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25356/
Test PASSed.

@asfgit asfgit closed this in 4b39fd1 Jan 10, 2015
asfgit pushed a commit that referenced this pull request Jan 10, 2015
The pull only fixes the parsing error and changes API to use tableIdentifier. Joining different catalog datasource related change is not done in this pull.

Author: Alex Liu <[email protected]>

Closes #3941 from alexliu68/SPARK-SQL-4943-3 and squashes the following commits:

343ae27 [Alex Liu] [SPARK-4943][SQL] refactoring according to review
29e5e55 [Alex Liu] [SPARK-4943][SQL] fix failed Hive CTAS tests
6ae77ce [Alex Liu] [SPARK-4943][SQL] fix TestHive matching error
3652997 [Alex Liu] [SPARK-4943][SQL] Allow table name having dot to support db/catalog ...

(cherry picked from commit 4b39fd1)
Signed-off-by: Michael Armbrust <[email protected]>

Conflicts:
	sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveQl.scala
	sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/CreateTableAsSelect.scala
@yhuai
Copy link
Contributor

yhuai commented Jan 13, 2015

@alexliu68 Is there any particular reason that org.apache.spark.sql.hive.HiveMetastoreCatalog#createTable does not take a tableIdentifier: Seq[String]?

@alexliu68
Copy link
Author

I only change selection to use tableIdentifier for joining tables. I keep createTable no change to minimize API changes. createTable could also use tableIdentifier to create catalog/cluster/database level table, but I leave it for future development if we need it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants