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.

Sign up for GitHub

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

Jump to bottom

[SPARK-5196][SQL] Support comment in Create Table Field DDL #3999

Closed
wants to merge 1 commit into from
Closed

[SPARK-5196][SQL] Support comment in Create Table Field DDL #3999

wants to merge 1 commit into from

Conversation

OopsOutOfMemory
Copy link
Contributor

Support comment in create a table field.
CREATE TEMPORARY TABLE people(name string comment "the name of a person")

@OopsOutOfMemory
Copy link
Contributor Author

This is the better way to implement the comment syntax.
Original implementation #3991 is to add a field comment to Expression, Atrribute, AtrributeReference which involved modifing too much files.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@OopsOutOfMemory OopsOutOfMemory mentioned this pull request Jan 12, 2015
Closed
@OopsOutOfMemory
Copy link
Contributor Author

Jenkins, test this please

@OopsOutOfMemory
Copy link
Contributor Author

@yhuai /cc @marmbrus
Could you review it ? After this, I'd like describe table command #3935 to support display the comment field of each column.

@OopsOutOfMemory
Copy link
Contributor Author

ping.

@yhuai
Copy link
Contributor

yhuai commented Jan 12, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Jan 12, 2015

Test build #25421 has started for PR 3999 at commit 81b8431.

  • This patch merges cleanly.

yhuai
yhuai reviewed Jan 12, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala
ident ~ dataType ~ (COMMENT ~> stringLit).? ^^ { case columnName ~ typ ~ cm =>
val metadata = new MetadataBuilder()
.putString(COMMENT.str.toLowerCase(), cm.getOrElse("")).build()
StructField(columnName, typ, true, metadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we do not define the metadata if a comment is not provided? I feel we are losing information if we use an empty string when a comment is not provided.

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 the test failure will be resolved once we do not create a metadata if there is no comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's fine.

@SparkQA
Copy link

SparkQA commented Jan 12, 2015

Test build #25421 has finished for PR 3999 at commit 81b8431.

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

@AmplabJenkins
Copy link

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

@OopsOutOfMemory
Copy link
Contributor Author

test this please

@OopsOutOfMemory
Copy link
Contributor Author

@yhuai /cc @marmbrus
Updated code.

@SparkQA
Copy link

SparkQA commented Jan 12, 2015

Test build #25424 has started for PR 3999 at commit c2b6f3f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 12, 2015

Test build #25424 has finished for PR 3999 at commit c2b6f3f.

  • 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/25424/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 13, 2015

Test build #25445 has started for PR 3999 at commit 92fa3eb.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 13, 2015

Test build #25445 has finished for PR 3999 at commit 92fa3eb.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

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

@SparkQA
Copy link

SparkQA commented Jan 13, 2015

Test build #25449 has started for PR 3999 at commit 1d0fbac.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 13, 2015

Test build #25449 has finished for PR 3999 at commit 1d0fbac.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

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

@SparkQA
Copy link

SparkQA commented Jan 13, 2015

Test build #25450 has started for PR 3999 at commit d2c4caf.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 13, 2015

Test build #25450 has finished for PR 3999 at commit d2c4caf.

  • 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/25450/
Test PASSed.

@OopsOutOfMemory
Copy link
Contributor Author

@yhuai /cc @marmbrus
Does this look OK to you?

@OopsOutOfMemory
Copy link
Contributor Author

ping

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25538 has started for PR 3999 at commit d5cba72.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25538 has finished for PR 3999 at commit d5cba72.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

yhuai
yhuai reviewed Jan 21, 2015
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/sources/TableScanSuite.scala
""".stripMargin)

val comments = sql("SELECT * FROM student").queryExecution.sparkPlan.
schema.fields.map { field =>
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 it is better to use queryExecution.executedPlan since it is the final physical. Also, this line is kind of long. How about we break it as follows.

val planned = sql("SELECT * FROM student").queryExecution.executedPlan
val comments = planned.schema.fields.map { ...

@yhuai
Copy link
Contributor

yhuai commented Jan 21, 2015

Just one minor comment. Other than that, LGTM.

@SparkQA
Copy link

SparkQA commented Jan 21, 2015

Test build #25876 has started for PR 3999 at commit de6daa7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 21, 2015

Test build #25876 has finished for PR 3999 at commit de6daa7.

  • 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/25876/
Test PASSed.

@OopsOutOfMemory
Copy link
Contributor Author

/cc @yhuai @marmbrus
Test Passed.
Is that OK with you?

@yhuai
Copy link
Contributor

yhuai commented Jan 21, 2015

LGTM

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25949 has started for PR 3999 at commit 1c71505.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25950 has started for PR 3999 at commit d1cfb0f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25949 has finished for PR 3999 at commit 1c71505.

  • 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/25949/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25950 has finished for PR 3999 at commit d1cfb0f.

  • 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/25950/
Test PASSed.

@OopsOutOfMemory
Copy link
Contributor Author

ping @marmbrus @yhuai
I think this is ready to go.

marmbrus
marmbrus reviewed Jan 29, 2015
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/sources/TableScanSuite.scala
else "NO_COMMENT"
}.mkString(",")

assert(comments == "SN,SA,NO_COMMENT")
Copy link
Contributor

Choose a reason for hiding this comment

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

Plain asserts should be avoided in tests as they give very little information when there is a failure. Instead use === or add a custom string that describes what went wrong as the second argument.

@SparkQA
Copy link

SparkQA commented Jan 30, 2015

Test build #26369 has started for PR 3999 at commit 77bf089.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 30, 2015

Test build #26371 has started for PR 3999 at commit 39150d4.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 30, 2015

Test build #26369 has finished for PR 3999 at commit 77bf089.

  • 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/26369/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 30, 2015

Test build #26371 has finished for PR 3999 at commit 39150d4.

  • 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/26371/
Test PASSed.

@OopsOutOfMemory
Copy link
Contributor Author

@marmbrus @rxin
Is this ready to merge?

marmbrus
marmbrus reviewed Feb 2, 2015
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/sources/TableScanSuite.scala
else "NO_COMMENT"
}.mkString(",")

assert(comments === "SN,SA,NO_COMMENT", "assert equals, display all comments of fields")
Copy link
Contributor

Choose a reason for hiding this comment

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

I will fix this up while merging, but I don't think it is useful to both use === and give a comment that doesn't include the actual answer. Ideally, when the test fails you want to see the answer that does not match the expected answer so you can figure out what went wrong without having to go and add println manually.

@asfgit asfgit closed this in 1b56f1d Feb 2, 2015
@OopsOutOfMemory OopsOutOfMemory deleted the meta_comment branch February 4, 2015 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@OopsOutOfMemory @AmplabJenkins @yhuai @SparkQA @marmbrus