Skip to content

Conversation

@yaooqinn
Copy link
Member

What changes were proposed in this pull request?

Display char/varchar in

  • DESC table
  • DESC column
  • SHOW CREATE TABLE

Why are the changes needed?

show the correct definition for users

Does this PR introduce any user-facing change?

yes, char/varchar column's will print char/varchar instead of string

How was this patch tested?

new tests

@SparkQA
Copy link

SparkQA commented Dec 23, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37900/

@SparkQA
Copy link

SparkQA commented Dec 23, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37900/

@yaooqinn
Copy link
Member Author

cc @cloud-fan @maropu @HyukjinKwon thanks a lot

@github-actions github-actions bot added the SQL label Dec 23, 2020
isExtended: Boolean) extends V2CommandExec {


override def producedAttributes: AttributeSet = outputSet
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we do this in V2CommandExec?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I agree. I was wondering if it is relevant to this PR.

}
}

test("DESCRIBE COLUMN w/ char/varchar") {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we add a TODO? eventually, v2 catalog can run these 2 commands as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any existing JIRA to track this, or I shall create one, then mark todo w/ it togeter

Copy link
Member Author

Choose a reason for hiding this comment

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

@SparkQA
Copy link

SparkQA commented Dec 23, 2020

Test build #133306 has finished for PR 30908 at commit ae1d059.

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

@SparkQA
Copy link

SparkQA commented Dec 23, 2020

Test build #133317 has finished for PR 30908 at commit bf6a173.

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

@SparkQA
Copy link

SparkQA commented Dec 23, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37910/

@SparkQA
Copy link

SparkQA commented Dec 23, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37910/

import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.encoders.RowEncoder
import org.apache.spark.sql.catalyst.expressions.{Attribute, GenericRowWithSchema}
import org.apache.spark.sql.catalyst.expressions.{Attribute, AttributeSet, GenericRowWithSchema}
Copy link
Member

@dongjoon-hyun dongjoon-hyun Dec 24, 2020

Choose a reason for hiding this comment

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

Do we need this addition in this PR? Otherwise, shall we avoid touching irrelevant files by reverting this change?

}
}

test("DESCRIBE TABLE w/ char/varchar") {
Copy link
Member

Choose a reason for hiding this comment

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

Please add SPARK-33892: prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

}
}

// TODO: Move this test to super after SPARK-33875 implements DESCRIBE COLUMN for v2
Copy link
Member

Choose a reason for hiding this comment

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

Please make this as IDed TODO like TODO(SPARK-33875) and add some description about Move this test... to SPARK-33875's description.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Please fix the linter error.

super.afterAll()
}

test("SHOW CREATE TABLE AS SERDE w/ char/varchar") {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@SparkQA
Copy link

SparkQA commented Dec 24, 2020

Test build #133332 has finished for PR 30908 at commit 2adf7f9.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master/3.1!

@cloud-fan cloud-fan closed this in 29cca68 Dec 24, 2020
cloud-fan pushed a commit that referenced this pull request Dec 24, 2020
### What changes were proposed in this pull request?

Display char/varchar in
  - DESC table
  - DESC column
  - SHOW CREATE TABLE

### Why are the changes needed?

show the correct definition for users

### Does this PR introduce _any_ user-facing change?

yes, char/varchar  column's will print char/varchar instead of string
### How was this patch tested?

new tests

Closes #30908 from yaooqinn/SPARK-33892.

Authored-by: Kent Yao <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 29cca68)
Signed-off-by: Wenchen Fan <[email protected]>
@SparkQA
Copy link

SparkQA commented Dec 24, 2020

Test build #133347 has finished for PR 30908 at commit 057f1ff.

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

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants