[SPARK-24268][SQL] Use datatype.simpleString in error messages #21321

mgaido91 · 2018-05-14T12:36:42Z

What changes were proposed in this pull request?

SPARK-22893 tried to unify error messages about dataTypes. Unfortunately, still many places were missing the simpleString method in other to have the same representation everywhere.

The PR unified the messages using alway the simpleString representation of the dataTypes in the messages.

How was this patch tested?

existing/modified UTs

maropu · 2018-05-14T14:28:08Z

jsonExpressions.scala also has the same issue?

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala

Line 756 in b6c50d7

s"A type of keys and values in map() must be string, but got ${m.dataType}")

Then, you need to update the output results in SQLQueryTestSuite.

SparkQA · 2018-05-14T15:36:34Z

Test build #90586 has finished for PR 21321 at commit ada7667.

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

mgaido91 · 2018-05-14T15:39:40Z

thanks @maropu I missed that one. I'll update it shortly, thanks.

SparkQA · 2018-05-15T13:53:05Z

Test build #90638 has finished for PR 21321 at commit e09026b.

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

mgaido91 · 2018-05-15T13:55:51Z

cc @gatorsmile

mgaido91 · 2018-05-25T13:36:17Z

kindly ping @gatorsmile

mgaido91 · 2018-06-12T14:52:14Z

kindly ping @gatorsmile

holdenk

Left a quick question for you from skimming this PR.

holdenk · 2018-06-13T16:41:41Z

mllib/src/main/scala/org/apache/spark/ml/feature/FeatureHasher.scala

        dataType.isInstanceOf[StringType] ||
        dataType.isInstanceOf[BooleanType],
-        s"FeatureHasher requires columns to be of NumericType, BooleanType or StringType. " +
+        s"FeatureHasher requires columns to be of ${NumericType.simpleString}, " +


In the original PR it doesn't seem like we rewrote the constant types only the dynamic ones (and this PR also doesn't seem to consistently rewrite the constant types referenced). What's the reason/how do you decide which ones you want to rewrite to ref simpleString?

I think this PR rewrites always constant type referenced. I am not sure why you are saying it is not. If I missed some places, then it was just because I haven't seen them.

I think that's my bad, looking through it I saw some raw type names but those are all in the suites which makes sense.

holdenk

Question about if we want to change to using SchemaUtils rather than just changing the error messages.

Some places which might have been missed in this PR under ml/ which I did a quick check with grep -r "Type" ./mllib/src/main/scala/org/apache/spark/ml |grep -i require (my bash regex might not be great but you get the idea):

./mllib/src/main/scala/org/apache/spark/ml/feature/DCT.scala: require(inputType.isInstanceOf[VectorUDT], s"Input type must be VectorUDT but got $inputType.")

./mllib/src/main/scala/org/apache/spark/ml/feature/Tokenizer.scala: require(inputType == StringType, s"Input type must be string type but got $inputType.")

StringIndexer.scala

If you have a chance to double check those that would be great!

holdenk · 2018-06-28T20:06:13Z

mllib/src/main/scala/org/apache/spark/ml/feature/FeatureHasher.scala

    fields.foreach { fieldSchema =>
      val dataType = fieldSchema.dataType
      val fieldName = fieldSchema.name
      require(dataType.isInstanceOf[NumericType] ||


Another option would be to use SchemaUtils checkColumnTypes here -- what do you think?

I couldn't do that because NumericType is an AbstractDataType and not a DataType so I couldn't use that method.

mgaido91 · 2018-06-29T08:38:57Z

Thanks for your kind and nice review @holdenk, I addressed the places I missed. Thank you!

SparkQA · 2018-06-29T12:29:08Z

Test build #92470 has finished for PR 21321 at commit bd8446c.

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

HyukjinKwon

Looks worth giving a try since it's an error message and we can revert anytime if we see some other concerns.

One thing I am hesitant is, IIRC @hvanhovell preferred catalogString. I will leave a cc for him.

HyukjinKwon · 2018-07-04T01:45:00Z

@mgaido91, mind rebasing this please? Let me just get this in.

mgaido91 · 2018-07-09T08:56:52Z

sorry for the late answer @HyukjinKwon. I just rebased it, thanks.

SparkQA · 2018-07-09T12:50:56Z

Test build #92747 has finished for PR 21321 at commit 781b64d.

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

HyukjinKwon · 2018-07-09T14:58:36Z

Merged to master.

gatorsmile · 2018-07-09T21:21:07Z

I think the fix is wrong. We should not use simpleString but catalogString, because simpleString will do the truncation.

gatorsmile · 2018-07-09T21:22:37Z

Let me revert the changes. Please re-submit the fix.

HyukjinKwon · 2018-07-10T01:12:58Z

That was the thing I was hesitant of. I was kind of confused recently about this because simpleString was used in some recent PRs even although I know catalogString is preferred. If this fix is "wrong", other PRs using simpleString all should be reverted though.

@mgaido91, mind opening a PR with catalogString again please?

gatorsmile · 2018-07-10T06:14:28Z

@HyukjinKwon The whole PR is doing the wrong things. That is why I reverted it. I do not want the others to follow this PR. For the other PRs whose main objective are not to use simpleString to improve the error message, we can submit follow-up PRs to fix them; otherwise, feel free to revert them WDYT?

HyukjinKwon · 2018-07-10T06:23:34Z

Ah, it's fine. This one is already reverted and I don't mind. Not a big deal actually. Glad that we are on the same page about catalogString anyway.

mgaido91 · 2018-07-10T08:54:39Z

@gatorsmile I used simpleString because in the previous JIRA which attempted to unify error messages simpleString was used, as well as I have always seen that method being used. Anyway, I am fine with catalogString too, but then we should change also the current places which are using simpleString, do we agree on this solution?

HyukjinKwon · 2018-07-10T09:35:36Z

For instance, #20064 at SPARK-22893, right? Let's fix them too. Yes, since this one is reverted, we should better stick to catalogString in all such places, which I believe we all agreed upon here now.

I only haven't left some comments about this so far only because my impression was that we were not yet sure between simpleString and catalogString in error messages.

Now it looks at least including me three committer prefers catalogString in error messages.

gatorsmile · 2018-07-10T16:18:24Z

catalogString is preferred in error messages.

[SPARK-24268][SQL] Use datatype.simpleString in error messages

ada7667

fix remaining issues

e09026b

holdenk reviewed Jun 13, 2018

View reviewed changes

holdenk reviewed Jun 28, 2018

View reviewed changes

address review findings

bd8446c

HyukjinKwon approved these changes Jun 30, 2018

View reviewed changes

Merge branch 'master' of github.com:apache/spark into SPARK-24268

781b64d

asfgit closed this in 1bd3d61 Jul 9, 2018

mgaido91 mentioned this pull request Jul 18, 2018

[SPARK-24268][SQL] Use datatype.catalogString in error messages #21804

Closed

[SPARK-24268][SQL] Use datatype.simpleString in error messages #21321

[SPARK-24268][SQL] Use datatype.simpleString in error messages #21321

Uh oh!

Conversation

mgaido91 commented May 14, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

maropu commented May 14, 2018

Uh oh!

SparkQA commented May 14, 2018

Uh oh!

mgaido91 commented May 14, 2018

Uh oh!

SparkQA commented May 15, 2018

Uh oh!

mgaido91 commented May 15, 2018

Uh oh!

mgaido91 commented May 25, 2018

Uh oh!

mgaido91 commented Jun 12, 2018

Uh oh!

holdenk left a comment

Choose a reason for hiding this comment

Uh oh!

holdenk Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

mgaido91 Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

holdenk Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

holdenk left a comment

Choose a reason for hiding this comment

Uh oh!

holdenk Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

mgaido91 Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

mgaido91 commented Jun 29, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 4, 2018

Uh oh!

mgaido91 commented Jul 9, 2018

Uh oh!

SparkQA commented Jul 9, 2018

Uh oh!

HyukjinKwon commented Jul 9, 2018

Uh oh!

gatorsmile commented Jul 9, 2018

Uh oh!

gatorsmile commented Jul 9, 2018

Uh oh!

HyukjinKwon commented Jul 10, 2018

Uh oh!

gatorsmile commented Jul 10, 2018

Uh oh!

HyukjinKwon commented Jul 10, 2018

Uh oh!

mgaido91 commented Jul 10, 2018

Uh oh!

HyukjinKwon commented Jul 10, 2018

Uh oh!

gatorsmile commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development