Skip to content

[SPARK-19666][SQL] Skip a property without getter in Java schema inference and allow empty bean in encoder creation #17013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
HyukjinKwon wants to merge 7 commits into from
Closed

[SPARK-19666][SQL] Skip a property without getter in Java schema inference and allow empty bean in encoder creation #17013

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9ad4789
Improve error message for invalid JavaBean
HyukjinKwon Feb 21, 2017
ae4c9aa
Address comments
HyukjinKwon Feb 21, 2017
91cee26
Allow setter only bean in java schema inference and empty bean in enc…
HyukjinKwon Feb 22, 2017
5808d71
Cleaner and comments
HyukjinKwon Feb 22, 2017
ed686fa
Cleaner tests
HyukjinKwon Feb 22, 2017
3604855
Address comments
HyukjinKwon Feb 22, 2017
ac5cc7d
Cleaner
HyukjinKwon Feb 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,18 @@ object JavaTypeInference {
val (valueDataType, nullable) = inferDataType(valueType)
(MapType(keyDataType, valueDataType, nullable), true)

case _ =>
case c =>
// 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 beanInfo = Introspector.getBeanInfo(typeToken.getRawType)
val properties = beanInfo.getPropertyDescriptors.filterNot(_.getName == "class")

@maropu maropu Feb 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to guard an empty properties? In JavaTypeInference#serializerFor https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala#L421, it takes the case into account.

@HyukjinKwon HyukjinKwon Feb 21, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, It seems loosely related with the JIRA here. I am fine with changing it if any committer could confirm. Otherwise, I would like to avoid a behaviour change here.

@srowen srowen Feb 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems like behavior should be the same in this case, in this same file. I think you could also improve that error handling.

Is there anywhere else in the code that checks the getter? is the error message consistent with that?

@HyukjinKwon HyukjinKwon Feb 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It seems like behavior should be the same in this case, in this same file. I think you could also improve that error handling.

The check he pointed out is related only with Encoders.bean(...) API as it seems only used in deserializerFor/serializerFor via ExpressionEncoder.scala#L85, which seems always executed after the codes I proposed to fix here.

The below codes just do not allow a bean with empty property after the properties without getters are filtered in JavaTypeInference.scala#L137 and it checks if they are empty in JavaTypeInference.scala#L421.

spark.createDataset(data, Encoders.bean(EmptyBean.class));

it throws an exception as below:

java.lang.UnsupportedOperationException: Cannot infer type for class EmptyBean because it is not bean-compliant
	at org.apache.spark.sql.catalyst.JavaTypeInference$.org$apache$spark$sql$catalyst$JavaTypeInference$$serializerFor(JavaTypeInference.scala:437)
	at org.apache.spark.sql.catalyst.JavaTypeInference$.serializerFor(JavaTypeInference.scala:342)

If we do not use Encoders.bean(...), then it allows empty schema as below:

spark.createDataFrame(data, EmptyBean.class);

For the sake of consistency, I guess we might have to consider removing the checks there , JavaTypeInference.scala#L421, and allowing this empty case because this seems possible in Scala API too as below:

import spark.implicits._

case class A()
spark.createDataset(Seq(A()))
spark.createDataFrame(Seq(A()))

Is there anywhere else in the code that checks the getter? is the error message consistent with that?

It seems there are several places

It seems the error message is consistent with this because the change I proposed here seems executed first before these places.

Java

spark.createDataFrame(data, BeanWithoutGetter.class);

spark.createDataset(data, Encoders.bean(BeanWithoutGetter.class));

Scala

 spark.createDataFrame(rdd, classOf[BeanWithoutGetter])

prints

java.lang.UnsupportedOperationException: Cannot infer type for class BeanWithoutGetter because property a does not have the getter
	at org.apache.spark.sql.catalyst.JavaTypeInference$$anonfun$2$$anonfun$3.apply(JavaTypeInference.scala:127)
	at org.apache.spark.sql.catalyst.JavaTypeInference$$anonfun$2$$anonfun$3.apply(JavaTypeInference.scala:127)

@HyukjinKwon HyukjinKwon Feb 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Honestly, I would like to fix the empty property change and sweep all related tests if there are (I guess this relates to #17013 (comment)) or avoid this change here because it seems loosely related.

@HyukjinKwon HyukjinKwon Feb 22, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(I editted the comments above for more details and preventing ambiguity.)

@cloud-fan cloud-fan Feb 22, 2017

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.

ideally a property without a getter is not a bean property, let's fix the empty property problem.

@HyukjinKwon HyukjinKwon Feb 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am sorry for the long comment that maybe a bit messed around. So, there are two code paths

  • JavaTypeInference.serializerFor/deserializerFor: non-empty required & getter/setter required

  • JavaTypeInference.inferDataType: empty one OK & getter only OK

Could I please ask which case you want just to double check?

@cloud-fan cloud-fan Feb 22, 2017

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 mean JavaTypeInference. Why we throw exception for a bean property without getter instead of not treating it as a property?

@HyukjinKwon HyukjinKwon Feb 22, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, sure. Then, let me ignore a property without the getter in JavaTypeInference.inferDataType, and allow empty property in JavaTypeInference.serializerFor/deserializerFor.

val fields = properties.map { property =>
val returnType = typeToken.method(property.getReadMethod).getReturnType
val readMethod = Option(property.getReadMethod).getOrElse {

@HyukjinKwon HyukjinKwon Feb 21, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This

May return null if the property can't be read.

throw new UnsupportedOperationException(
s"Cannot infer type for class ${c.getName} " +
s"because property ${property.getName} does not have the getter")
}
val returnType = typeToken.method(readMethod).getReturnType
val (dataType, nullable) = inferDataType(returnType)
new StructField(property.getName, dataType, nullable)
}
Expand Down
15 changes: 15 additions & 0 deletions sql/core/src/test/java/test/org/apache/spark/sql/JavaDataFrameSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ public List<String> getD() {
public BigInteger getE() { return e; }
}

public static class BeanWithoutGetter implements Serializable {
private String a;

public void setA(String a) {
this.a = a;
}
}

void validateDataFrameWithBeans(Bean bean, Dataset<Row> df) {
StructType schema = df.schema();
Assert.assertEquals(new StructField("a", DoubleType$.MODULE$, false, Metadata.empty()),
Expand Down Expand Up @@ -397,4 +405,11 @@ public void testBloomFilter() {
Assert.assertTrue(filter4.mightContain(i * 3));
}
}

@Test(expected = UnsupportedOperationException.class)
public void testBeanWithoutGetter() {
BeanWithoutGetter bean = new BeanWithoutGetter();
List<BeanWithoutGetter> data = Arrays.asList(bean);
spark.createDataFrame(data, BeanWithoutGetter.class);
}
}