[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-25 Thread cloud-fan
Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r135377225
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -118,6 +119,10 @@ object JavaTypeInference {
 val (valueDataType, nullable) = inferDataType(valueType, 
seenTypeSet)
 (MapType(keyDataType, valueDataType, nullable), true)
 
+  case other if other.isEnum =>
+(StructType(Seq(StructField(typeToken.getRawType.getSimpleName,
--- End diff --

why we map enum to struct type? shouldn't enum always have a single field?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-25 Thread asfgit
Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134622727
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -154,13 +154,13 @@ case class StaticInvoke(
 val evaluate = if (returnNullable) {
   if (ctx.defaultValue(dataType) == "null") {
 s"""
-  ${ev.value} = $callFunc;
+  ${ev.value} = (($javaType) ($callFunc));
--- End diff --

from janino documentation: "Type arguments: Are parsed, but otherwise 
ignored. The most significant restriction that follows is that you must cast 
return values from method invocations, e.g. "(String) myMap.get(key)" 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134480556
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -154,13 +154,13 @@ case class StaticInvoke(
 val evaluate = if (returnNullable) {
   if (ctx.defaultValue(dataType) == "null") {
 s"""
-  ${ev.value} = $callFunc;
+  ${ev.value} = (($javaType) ($callFunc));
--- End diff --

explicitly cast value to needed type, because without this generated code 
didn't compile with something like "cannot assign value of type Enum to 
%RealEnumClassName%


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134479564
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala
 ---
@@ -81,9 +81,19 @@ object ExpressionEncoder {
   ClassTag[T](cls))
   }
 
+  def javaEnumSchema[T](beanClass: Class[T]): DataType = {
+StructType(Seq(StructField("enum",
+  StructType(Seq(StructField(beanClass.getSimpleName, StringType, 
nullable = false))),
+  nullable = false)))
+  }
+
   // TODO: improve error message for java bean encoder.
   def javaBean[T](beanClass: Class[T]): ExpressionEncoder[T] = {
-val schema = JavaTypeInference.inferDataType(beanClass)._1
+val schema = if (beanClass.isEnum) {
+  javaEnumSchema(beanClass)
--- End diff --

If we use enum as top level object, we need another level of structType for 
it to be compatible with our ser/de structure


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134478774
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -429,6 +464,11 @@ object JavaTypeInference {
 valueNullable = true
   )
 
+case other if other.isEnum =>
+  CreateNamedStruct(expressions.Literal("enum") ::
+  StaticInvoke(JavaTypeInference.getClass, StringType, 
"serializeEnumName",
+  expressions.Literal.create(other.getName, StringType) :: 
inputObject :: Nil) :: Nil)
+
--- End diff --

we pass enum class name via literal to serializer


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134478386
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -345,6 +356,30 @@ object JavaTypeInference {
 }
   }
 
+  /** Returns a mapping from enum value to int for given enum type */
+  def enumSerializer[T <: Enum[T]](enum: Class[T]): T => UTF8String = {
+assert(enum.isEnum)
+inputObject: T =>
+  UTF8String.fromString(inputObject.name())
+  }
+
+  /** Returns value index for given enum type and value */
+  def serializeEnumName[T <: Enum[T]](enum: UTF8String, inputObject: T): 
UTF8String = {
+
enumSerializer(Utils.classForName(enum.toString).asInstanceOf[Class[T]])(inputObject)
+  }
+
+  /** Returns a mapping from int to enum value for given enum type */
+  def enumDeserializer[T <: Enum[T]](enum: Class[T]): InternalRow => T = {
+assert(enum.isEnum)
+value: InternalRow =>
+  Enum.valueOf(enum, value.getUTF8String(0).toString)
--- End diff --

Enum.valueOf uses cached string->value map


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134477534
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -345,6 +356,30 @@ object JavaTypeInference {
 }
   }
 
+  /** Returns a mapping from enum value to int for given enum type */
+  def enumSerializer[T <: Enum[T]](enum: Class[T]): T => UTF8String = {
+assert(enum.isEnum)
+inputObject: T =>
+  UTF8String.fromString(inputObject.name())
--- End diff --

we use enum constant name as field value


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134477280
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -345,6 +356,30 @@ object JavaTypeInference {
 }
   }
 
+  /** Returns a mapping from enum value to int for given enum type */
+  def enumSerializer[T <: Enum[T]](enum: Class[T]): T => UTF8String = {
+assert(enum.isEnum)
+inputObject: T =>
+  UTF8String.fromString(inputObject.name())
+  }
+
+  /** Returns value index for given enum type and value */
+  def serializeEnumName[T <: Enum[T]](enum: UTF8String, inputObject: T): 
UTF8String = {
+
enumSerializer(Utils.classForName(enum.toString).asInstanceOf[Class[T]])(inputObject)
+  }
--- End diff --

Utils.classForName delegates to Class.forName, which operates on native 
level, so additional optimizations like caching are not required


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134475878
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -303,6 +309,11 @@ object JavaTypeInference {
   keyData :: valueData :: Nil,
   returnNullable = false)
 
+  case other if other.isEnum =>
+StaticInvoke(JavaTypeInference.getClass, ObjectType(other), 
"deserializeEnumName",
+  expressions.Literal.create(other.getEnumConstants.apply(0), 
ObjectType(other))
+:: getPath :: Nil)
+
--- End diff --

we pass literal value of first enum constant to resolve type parameter of 
deserializeEnumName method


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-22 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r134475250
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -118,6 +119,10 @@ object JavaTypeInference {
 val (valueDataType, nullable) = inferDataType(valueType, 
seenTypeSet)
 (MapType(keyDataType, valueDataType, nullable), true)
 
+  case other if other.isEnum =>
+(StructType(Seq(StructField(typeToken.getRawType.getSimpleName,
+  StringType, nullable = false))), true)
+
--- End diff --

We use struct type with string field to store enum type and it's value


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-15 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r133194603
  
--- Diff: 
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionInfo.java
 ---
@@ -79,7 +79,7 @@ public ExpressionInfo(
 assert name != null;
 assert arguments != null;
 assert examples != null;
-assert examples.isEmpty() || examples.startsWith("\n
Examples:");
+assert examples.isEmpty() || 
examples.startsWith(System.lineSeparator() + "Examples:");
--- End diff --

ok, I got rid of it


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-14 Thread srowen
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r132983668
  
--- Diff: 
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionInfo.java
 ---
@@ -79,7 +79,7 @@ public ExpressionInfo(
 assert name != null;
 assert arguments != null;
 assert examples != null;
-assert examples.isEmpty() || examples.startsWith("\n
Examples:");
+assert examples.isEmpty() || 
examples.startsWith(System.lineSeparator() + "Examples:");
--- End diff --

I don't think we support Windows for dev. This assertion should probably be 
weakened anyway but that's a separate issue from this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-10 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r132489991
  
--- Diff: 
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionInfo.java
 ---
@@ -79,7 +79,7 @@ public ExpressionInfo(
 assert name != null;
 assert arguments != null;
 assert examples != null;
-assert examples.isEmpty() || examples.startsWith("\n
Examples:");
+assert examples.isEmpty() || 
examples.startsWith(System.lineSeparator() + "Examples:");
--- End diff --

No, but without it it's not possible to run tests if you have different 
line separators (on windows for example)



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-08-10 Thread HyukjinKwon
Github user HyukjinKwon commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r132468980
  
--- Diff: 
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionInfo.java
 ---
@@ -79,7 +79,7 @@ public ExpressionInfo(
 assert name != null;
 assert arguments != null;
 assert examples != null;
-assert examples.isEmpty() || examples.startsWith("\n
Examples:");
+assert examples.isEmpty() || 
examples.startsWith(System.lineSeparator() + "Examples:");
--- End diff --

I guess this one is not related?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-07-03 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r125360956
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -344,6 +352,28 @@ object JavaTypeInference {
 }
   }
 
+  /** Returns a mapping from enum value to int for given enum type */
+  def enumSerializer[T](enum: Class[T]): T => Int = {
+assert(enum.isEnum)
+enum.getEnumConstants.toList.zipWithIndex.toMap
+  }
+
+  /** Returns value index for given enum type and value */
+  def serializeEnumName[T](enum: UTF8String, inputObject: T): Int = {
+
enumSerializer(Utils.classForName(enum.toString).asInstanceOf[Class[T]])(inputObject)
+  }
+
+  /** Returns a mapping from int to enum value for given enum type */
+  def enumDeserializer[T](enum: Class[T]): Int => T = {
--- End diff --

But it's still not very good, because we have to lookup enum by class name 
for each object we serialize/deserialize. Do you have any ideas?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-07-03 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r125360539
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -344,6 +352,28 @@ object JavaTypeInference {
 }
   }
 
+  /** Returns a mapping from enum value to int for given enum type */
+  def enumSerializer[T](enum: Class[T]): T => Int = {
+assert(enum.isEnum)
+enum.getEnumConstants.toList.zipWithIndex.toMap
+  }
+
+  /** Returns value index for given enum type and value */
+  def serializeEnumName[T](enum: UTF8String, inputObject: T): Int = {
+
enumSerializer(Utils.classForName(enum.toString).asInstanceOf[Class[T]])(inputObject)
+  }
+
+  /** Returns a mapping from int to enum value for given enum type */
+  def enumDeserializer[T](enum: Class[T]): Int => T = {
--- End diff --

Good point. But if we use string values, there is already a hashmap inside 
enum implementation, accessible via valueOf.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-07-03 Thread mike0sv
Github user mike0sv commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r125358120
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -127,19 +128,24 @@ object JavaTypeInference {
 
 // TODO: we should only collect properties that have getter and 
setter. However, some tests
 // pass in scala case class as java bean class which doesn't have 
getter and setter.
-val properties = getJavaBeanReadableProperties(other)
-val fields = properties.map { property =>
-  val returnType = 
typeToken.method(property.getReadMethod).getReturnType
-  val (dataType, nullable) = inferDataType(returnType, seenTypeSet 
+ other)
-  new StructField(property.getName, dataType, nullable)
+if (typeToken.getRawType.isEnum) {
--- End diff --

Probably, but int is cheaper. However, if we use string, there will be a 
possibility for meaningful queries. Also, there are strange cases with complex 
enums with different fields, even with complex types. I'd say string with 
constant name is enough, thou


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-07-03 Thread srowen
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r125346554
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -127,19 +128,24 @@ object JavaTypeInference {
 
 // TODO: we should only collect properties that have getter and 
setter. However, some tests
 // pass in scala case class as java bean class which doesn't have 
getter and setter.
-val properties = getJavaBeanReadableProperties(other)
-val fields = properties.map { property =>
-  val returnType = 
typeToken.method(property.getReadMethod).getReturnType
-  val (dataType, nullable) = inferDataType(returnType, seenTypeSet 
+ other)
-  new StructField(property.getName, dataType, nullable)
+if (typeToken.getRawType.isEnum) {
--- End diff --

Could this be a `case rawType if rawType.isEnum =>` case instead?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-07-03 Thread srowen
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r125346872
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -127,19 +128,24 @@ object JavaTypeInference {
 
 // TODO: we should only collect properties that have getter and 
setter. However, some tests
 // pass in scala case class as java bean class which doesn't have 
getter and setter.
-val properties = getJavaBeanReadableProperties(other)
-val fields = properties.map { property =>
-  val returnType = 
typeToken.method(property.getReadMethod).getReturnType
-  val (dataType, nullable) = inferDataType(returnType, seenTypeSet 
+ other)
-  new StructField(property.getName, dataType, nullable)
+if (typeToken.getRawType.isEnum) {
--- End diff --

I'd also imagine the most natural type for an enum is a string, not a 
struct containing an int, but I haven't maybe thought that through


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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



[GitHub] spark pull request #18488: [SPARK-21255][SQL][WIP] Fixed NPE when creating e...

2017-07-03 Thread srowen
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/18488#discussion_r125346760
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
 ---
@@ -344,6 +352,28 @@ object JavaTypeInference {
 }
   }
 
+  /** Returns a mapping from enum value to int for given enum type */
+  def enumSerializer[T](enum: Class[T]): T => Int = {
+assert(enum.isEnum)
+enum.getEnumConstants.toList.zipWithIndex.toMap
+  }
+
+  /** Returns value index for given enum type and value */
+  def serializeEnumName[T](enum: UTF8String, inputObject: T): Int = {
+
enumSerializer(Utils.classForName(enum.toString).asInstanceOf[Class[T]])(inputObject)
+  }
+
+  /** Returns a mapping from int to enum value for given enum type */
+  def enumDeserializer[T](enum: Class[T]): Int => T = {
--- End diff --

The enum's `values()` value is already effectively a mapping from int to 
enum values -- much cheaper to access?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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