-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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-5135][SQL] Add support for describe [extended] table to DDL in SQLContext #3935
Conversation
Can one of the admins verify this patch? |
e.g.
|
ok to test |
you'll also need to add test cases |
Test build #25179 has started for PR 3935 at commit
|
Test build #25179 has finished for PR 3935 at commit
|
Test FAILed. |
@@ -328,18 +328,19 @@ class SQLContext(@transient val sparkContext: SparkContext) | |||
def numPartitions = self.numShufflePartitions | |||
|
|||
def strategies: Seq[Strategy] = | |||
extraStrategies ++ ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: a :: b :: c :: Nil
is exact the same with Seq(a, b, c)
. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I just make this same as hive context way.
@chenghao-intel
This test failed with Refer to this link for build results (access rights to CI server needed):
no rights to CL server ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bug of error reporting, anyway, the root reason is it failed in the code style checking.
/home/jenkins/workspace/SparkPullRequestBuilder/sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala:198:21: Insert a space after the start of the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revert SQLContext
in this PR? Since the change is equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok, it can at least save some space since ::
is 2 chars, ,
is 1 char.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @chenghao-intel said, do you mind not changing this part? It would be great if the PR focuses on what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chenghao-intel @rxin
ok, got it. I will change this.
That's a nice feature in general, I agree with @marmbrus , we do need a test suite for this. |
@chenghao-intel @marmbrus |
ok to test |
Test build #25198 has started for PR 3935 at commit
|
Test build #25198 has finished for PR 3935 at commit
|
Test PASSed. |
|
||
override def run(sqlContext: SQLContext) = { | ||
val rows = new ArrayBuffer[Row]() | ||
rows += Row("# col_name", "data_type", "comment") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"# col_name", "data_type", "comment"
is the output field name, we'd better not take that as part of the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chenghao-intel
ok, I will remove it.
If future support partition table
here, I will add it like hive does:
# Partition Information
# col_name data_type comment
patition_col_name string None
Test build #25216 has started for PR 3935 at commit
|
Test build #25216 has finished for PR 3935 at commit
|
Test PASSed. |
Test build #25226 has started for PR 3935 at commit
|
Test build #25226 has finished for PR 3935 at commit
|
Test FAILed. |
@scwf @chenghao-intel |
test this please. |
Test build #25227 has started for PR 3935 at commit
|
Test build #25229 has started for PR 3935 at commit
|
Test build #25639 has started for PR 3935 at commit
|
Test build #25639 has finished for PR 3935 at commit
|
Test FAILed. |
@rxin The rebase may have some problems, cause changed 278 files ? how do I revert it ? |
Not sure - do you have a backup? Maybe just take a diff and apply the diff on master. |
Test build #25651 has started for PR 3935 at commit
|
Test build #25651 has finished for PR 3935 at commit
|
Test FAILed. |
9efdf35
to
9d22708
Compare
Test build #25654 has started for PR 3935 at commit
|
@OopsOutOfMemory, you need revert unnecessary changes |
9d22708
to
5b7ae19
Compare
Test build #25655 has started for PR 3935 at commit
|
Test build #25654 has finished for PR 3935 at commit
|
Test PASSed. |
Test build #25655 has finished for PR 3935 at commit
|
Test PASSed. |
Test build #25665 has started for PR 3935 at commit
|
Test build #25665 has finished for PR 3935 at commit
|
Test PASSed. |
val ddlPlan = ddlParser(sqlText) | ||
val basicPlan = try { | ||
HiveQl.parseSql(sqlText) | ||
}catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a space here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it's ok i will fix it myself
[SPARK-5135][SQL] Add support for describe [extended] table to DDL in SQLContext Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/types/dataTypes.scala
Thanks. I resolved the conflict and pushed a PR #4127 |
Thanks @rxin |
Yea we can close this one. Will merge that one when tests pass. |
RT.
issue is here : https://issues.apache.org/jira/browse/SPARK-5135