[GitHub] spark pull request #21321: [SPARK-24268][SQL] Use datatype.simpleString in e...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/21321


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21321: [SPARK-24268][SQL] Use datatype.simpleString in e...

[mgaido91]

Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/21321#discussion_r199091397
  
--- Diff: 
mllib/src/main/scala/org/apache/spark/ml/feature/FeatureHasher.scala ---
@@ -208,7 +208,8 @@ class FeatureHasher(@Since("2.3.0") override val uid: 
String) extends Transforme
   require(dataType.isInstanceOf[NumericType] ||
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21321: [SPARK-24268][SQL] Use datatype.simpleString in e...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/21321#discussion_r198968646
  
--- Diff: 
mllib/src/main/scala/org/apache/spark/ml/feature/FeatureHasher.scala ---
@@ -208,7 +208,8 @@ class FeatureHasher(@Since("2.3.0") override val uid: 
String) extends Transforme
   require(dataType.isInstanceOf[NumericType] ||
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21321: [SPARK-24268][SQL] Use datatype.simpleString in e...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/21321#discussion_r198964798
  
--- Diff: 
mllib/src/main/scala/org/apache/spark/ml/feature/FeatureHasher.scala ---
@@ -208,7 +208,8 @@ class FeatureHasher(@Since("2.3.0") override val uid: 
String) extends Transforme
   require(dataType.isInstanceOf[NumericType] ||
 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}, " +
--- End diff --

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.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21321: [SPARK-24268][SQL] Use datatype.simpleString in e...

[mgaido91]

Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/21321#discussion_r195344259
  
--- Diff: 
mllib/src/main/scala/org/apache/spark/ml/feature/FeatureHasher.scala ---
@@ -208,7 +208,8 @@ class FeatureHasher(@Since("2.3.0") override val uid: 
String) extends Transforme
   require(dataType.isInstanceOf[NumericType] ||
 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}, " +
--- End diff --

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.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21321: [SPARK-24268][SQL] Use datatype.simpleString in e...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/21321#discussion_r195156323
  
--- Diff: 
mllib/src/main/scala/org/apache/spark/ml/feature/FeatureHasher.scala ---
@@ -208,7 +208,8 @@ class FeatureHasher(@Since("2.3.0") override val uid: 
String) extends Transforme
   require(dataType.isInstanceOf[NumericType] ||
 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}, " +
--- End diff --

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?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21321: [SPARK-24268][SQL] Use datatype.simpleString in e...

[mgaido91]

GitHub user mgaido91 opened a pull request:

https://github.com/apache/spark/pull/21321

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

## 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


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/mgaido91/spark SPARK-24268

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/21321.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #21321


commit ada7667a9f8872f373bc789a7eb0c84987642314
Author: Marco Gaido 
Date:   2018-05-05T15:19:45Z

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




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org