Skip to content

[SPARK-3892][SQL] remove redundant type name - #2747

Closed
adrian-wang wants to merge 2 commits into
apache:masterfrom
adrian-wang:typename
Closed

[SPARK-3892][SQL] remove redundant type name#2747
adrian-wang wants to merge 2 commits into
apache:masterfrom
adrian-wang:typename

Conversation

@adrian-wang

@adrian-wang adrian-wang commented Oct 10, 2014

Copy link
Copy Markdown
Contributor

No description provided.

@AmplabJenkins

AmplabJenkins commented Oct 10, 2014

Copy link
Copy Markdown

Can one of the admins verify this patch?

@SparkQA

SparkQA commented Oct 10, 2014

Copy link
Copy Markdown

QA tests have started for PR 2747 at commit fbaf340.

  • This patch merges cleanly.

@SparkQA

SparkQA commented Oct 10, 2014

Copy link
Copy Markdown

QA tests have finished for PR 2747 at commit fbaf340.

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

@AmplabJenkins

AmplabJenkins commented Oct 10, 2014

Copy link
Copy Markdown

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

liancheng
liancheng reviewed Oct 10, 2014
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/dataTypes.scala Outdated

@liancheng liancheng Oct 10, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to just remove this method since it's not used anywhere. Another important reason is that the DataType class hierarchy is now part of the public API, which should be minimized for safe :)

@liancheng liancheng Oct 10, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be honest, this method is there totally because I forgot to remove it... Otherwise at least object ArrayType and object StructType should have their own versions of typeName.

@adrian-wang adrian-wang Oct 10, 2014

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually they do have... should I remove them?

@liancheng liancheng Oct 11, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yea, please go ahead :)

@SparkQA

SparkQA commented Oct 11, 2014

Copy link
Copy Markdown

QA tests have started for PR 2747 at commit 2824216.

  • This patch merges cleanly.

@SparkQA

SparkQA commented Oct 11, 2014

Copy link
Copy Markdown

QA tests have finished for PR 2747 at commit 2824216.

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

@AmplabJenkins

AmplabJenkins commented Oct 11, 2014

Copy link
Copy Markdown

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

@liancheng

liancheng commented Oct 11, 2014

Copy link
Copy Markdown
Contributor

This LGTM. Please rename the PR title to reflect the actual changes.

@adrian-wang adrian-wang changed the title [SPARK-3892][SQL] Map type should have typeName [SPARK-3892][SQL] remove redundant type name Oct 11, 2014
@marmbrus

marmbrus commented Oct 13, 2014

Copy link
Copy Markdown
Contributor

Thanks! Merged to master.

@asfgit asfgit closed this in 46db277 Oct 13, 2014
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

Development

Successfully merging this pull request may close these issues.

5 participants

@adrian-wang @AmplabJenkins @SparkQA @liancheng @marmbrus