Skip to content

Conversation

@mgaido91
Copy link
Contributor

What changes were proposed in this pull request?

As stated in #21321, in the error messages we should use catalogString. This is not the case, as SPARK-22893 used simpleString in order to have the same representation everywhere and it missed some places.

The PR unifies the messages using alway the catalogString representation of the dataTypes in the messages.

How was this patch tested?

existing/modified UTs

@mgaido91
Copy link
Contributor Author

cc @gatorsmile @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Jul 18, 2018

Test build #93236 has finished for PR 21804 at commit eb78665.

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

@SparkQA
Copy link

SparkQA commented Jul 18, 2018

Test build #93237 has finished for PR 21804 at commit aeb1766.

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

@SparkQA
Copy link

SparkQA commented Jul 19, 2018

Test build #93252 has finished for PR 21804 at commit 5ffc793.

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



private[sql] object NumericType extends AbstractDataType {
private[spark] object NumericType extends AbstractDataType {
Copy link
Member

Choose a reason for hiding this comment

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

(This is just a question...) Is it ok for some types to have private[spark] and the others to have private[sql]? I feel a little inconsistent policy for that. Since the other components (e.g., ml and mllib) depend on the sql type system, is it bad to make all the modifiers in their types private[spark] consistently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine changing them all to private[spark]. @gatorsmile @HyukjinKwon what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

To me, let's leave them out for now, and fix or discuss when the change is actually needed later.

@SparkQA
Copy link

SparkQA commented Jul 19, 2018

Test build #93276 has finished for PR 21804 at commit 3c8eb98.

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

@SparkQA
Copy link

SparkQA commented Jul 19, 2018

Test build #93285 has finished for PR 21804 at commit 20ee744.

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

@gatorsmile
Copy link
Member

LGTM

Thanks! Merged to master.

@asfgit asfgit closed this in a5925c1 Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants