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-19272][SQL] Remove the param viewOriginalText from CatalogTable #16679

Closed
wants to merge 3 commits into from

Conversation

jiangxb1987
Copy link
Contributor

What changes were proposed in this pull request?

Hive will expand the view text, so it needs 2 fields: originalText and viewText. Since we don't expand the view text, but only add table properties, perhaps only a single field viewText is enough in CatalogTable.

This PR brought in the following changes:

  1. Remove the param viewOriginalText from CatalogTable;
  2. Update the output of command DescribeTableCommand.

How was this patch tested?

Tested by exsiting test cases, also updated the failed test cases.

@jiangxb1987
Copy link
Contributor Author

cc @cloud-fan

@@ -528,8 +528,10 @@ case class DescribeTableCommand(
private def describeViewInfo(metadata: CatalogTable, buffer: ArrayBuffer[Row]): Unit = {
append(buffer, "", "", "")
append(buffer, "# View Information", "", "")
append(buffer, "View Original Text:", metadata.viewOriginalText.getOrElse(""), "")
append(buffer, "View Expanded Text:", metadata.viewText.getOrElse(""), "")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not Expanded

@SparkQA
Copy link

SparkQA commented Jan 23, 2017

Test build #71835 has finished for PR 16679 at commit 7be0477.

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

@@ -426,7 +426,6 @@ private[hive] class HiveClientImpl(
// in the function toHiveTable.
properties = properties.filter(kv => kv._1 != "comment" && kv._1 != "EXTERNAL"),
comment = properties.get("comment"),
viewOriginalText = Option(h.getViewOriginalText),
viewText = Option(h.getViewExpandedText),
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment in this line to explain why we are using h.getViewExpandedText instead of h.getViewOriginalText?

@SparkQA
Copy link

SparkQA commented Jan 23, 2017

Test build #71868 has finished for PR 16679 at commit 882f76b.

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

viewOriginalText = Option(h.getViewOriginalText),
// In older versions of Spark(before 2.2.0), we expand the view original text and store
// that into `viewExpandedText`, and that should be used in view resolution. So we get
// `viewExpandedText` instead of `viewOriginalText` for viewText here.
Copy link
Member

Choose a reason for hiding this comment

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

BTW, do we have a test case to cover the backward compatibility? That means, Spark 2.2.0 still can use/call the view created by Spark 2.1.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, we do have such a test

@SparkQA
Copy link

SparkQA commented Jan 24, 2017

Test build #71900 has finished for PR 16679 at commit b5a48da.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in 3bdf3ee Jan 24, 2017
@jiangxb1987 jiangxb1987 deleted the catalogTable branch January 25, 2017 03:07
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
…able`

## What changes were proposed in this pull request?

Hive will expand the view text, so it needs 2 fields: originalText and viewText. Since we don't expand the view text, but only add table properties, perhaps only a single field `viewText` is enough in CatalogTable.

This PR brought in the following changes:
1. Remove the param `viewOriginalText` from `CatalogTable`;
2. Update the output of command `DescribeTableCommand`.

## How was this patch tested?

Tested by exsiting test cases, also updated the failed test cases.

Author: jiangxingbo <jiangxb1987@gmail.com>

Closes apache#16679 from jiangxb1987/catalogTable.
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
…able`

## What changes were proposed in this pull request?

Hive will expand the view text, so it needs 2 fields: originalText and viewText. Since we don't expand the view text, but only add table properties, perhaps only a single field `viewText` is enough in CatalogTable.

This PR brought in the following changes:
1. Remove the param `viewOriginalText` from `CatalogTable`;
2. Update the output of command `DescribeTableCommand`.

## How was this patch tested?

Tested by exsiting test cases, also updated the failed test cases.

Author: jiangxingbo <jiangxb1987@gmail.com>

Closes apache#16679 from jiangxb1987/catalogTable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants