Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-16691][SQL] move BucketSpec to catalyst module and use it in CatalogTable #14331

Closed
wants to merge 3 commits into from

Conversation

cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

It's weird that we have BucketSpec to abstract bucket info, but don't use it in CatalogTable. This PR moves BucketSpec into catalyst module.

How was this patch tested?

existing tests.

@cloud-fan
Copy link
Contributor Author

cc @yhuai @liancheng

@@ -764,10 +761,7 @@ private[hive] class HiveClientImpl(
hiveTable.setFields(schema.asJava)
}
hiveTable.setPartCols(partCols.asJava)
// TODO: set sort columns here too
hiveTable.setBucketCols(table.bucketColumnNames.asJava)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't support bucketed hive table now, and I think we never will, because we have different hash implementation.

@SparkQA
Copy link

SparkQA commented Jul 24, 2016

Test build #62763 has finished for PR 14331 at commit beefff2.

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

@gatorsmile
Copy link
Member

LGTM

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62776 has finished for PR 14331 at commit 59321d0.

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

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62777 has finished for PR 14331 at commit cc63f01.

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

}
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Remove extra newline.

@liancheng
Copy link
Contributor

Overall LGTM, a few minor comments.

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62798 has finished for PR 14331 at commit 069419d.

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

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62821 has finished for PR 14331 at commit b7b9946.

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

@liancheng
Copy link
Contributor

Merging to master, thanks!

@asfgit asfgit closed this in 64529b1 Jul 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants