You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by hamishmorgan <gi...@git.apache.org> on 2016/11/27 15:23:56 UTC
[GitHub] spark pull request #16022: [SPARK-18598][SQL] Fixed inconsistent bean proper...
GitHub user hamishmorgan opened a pull request:
https://github.com/apache/spark/pull/16022
[SPARK-18598][SQL] Fixed inconsistent bean property inference
## What changes were proposed in this pull request?
`JavaTypeInference#inferDataType` used a different bean property filter from `serializerFor` and `deserializerFor`. While `inferDataType` required properties to simple not be named "class", while the others required properties to have associated accessor and mutator. This caused an inconsistency in the `ExpressionEncoder`, resulting in an `AssertionError` when Datasets are operated on. (see https://issues.apache.org/jira/browse/SPARK-18598)
The PR patches `JavaTypeInference` to _always_ require properties to have an associated accessor and mutator.
## How was this patch tested?
Added a test case for the change.
The larger, upstream issue (of AssertionError being thrown) has been tested with an integration test (as seen in the JIRA description) but I haven't submitted that.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hamishmorgan/spark SPARK-18598
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16022.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 #16022
----
commit 3e5eecaa6dcd0997f3e9805e2f19a0ecf16f8696
Author: Hamish Morgan <ha...@gmail.com>
Date: 2016-11-27T13:38:24Z
[SPARK-18598][SQL] Added failing until test, demonstrating problem cause
Added license comment to new test files
Fixed indentation in new tests
commit 33d25c668bc3d47a426a5ce26a43c54195a4a39e
Author: Hamish Morgan <ha...@gmail.com>
Date: 2016-11-27T13:39:29Z
[SPARK-18598][SQL] Added fix inconsistent bean field counts
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16022
Given the comment it sounds like this change could cause tests to fail. We'll find out shortly. It seems like a good change because indeed bean properties are defined by getters and/or setters so it seems like the same definition should be used here.
@cloud-fan will know more.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16022
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/69203/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by hamishmorgan <gi...@git.apache.org>.
Github user hamishmorgan commented on the issue:
https://github.com/apache/spark/pull/16022
Probably. Worst case, we should be able to at least fail faster with more helpful message. And maybe there's something better. I'll have a poke around and see.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16022
Jenkins test this please
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16022
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16022
Can one of the admins verify this patch?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/16022
Can we fix that specific bug than taking away the ability to infer just case classes?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/16022
We can't do this since it breaks compatibility now.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16022
**[Test build #69203 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/69203/consoleFull)** for PR 16022 at commit [`7c9a630`](https://github.com/apache/spark/commit/7c9a6307089f9ba2a84bf4bd31552d853050bd65).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by hamishmorgan <gi...@git.apache.org>.
Github user hamishmorgan commented on the issue:
https://github.com/apache/spark/pull/16022
Thanks @srowen - I see you were right. Not sure how proceed. Some options:
* Update failing tests to load their data a different way.
* Update the relevant beans (used in the tests) to include accessors and mutators.
* Define a bean as _any_ property, irrespective of accessors. This gives up on the bean contract, which maybe doesn't make sense in Scala anyway.
* Hack something into the ExpressionEncoder to make the resultant Datasets less broken, but that's not really dealing with the root cause.
Either way, I think this becomes a breaking change. Thoughts?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16022
**[Test build #69203 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/69203/consoleFull)** for PR 16022 at commit [`7c9a630`](https://github.com/apache/spark/commit/7c9a6307089f9ba2a84bf4bd31552d853050bd65).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16022
Yes, all but one of the failures arise because `LabeledPoint` is used as a bean class but it's a case class. It would be straightforward to add `def getFeatures = features` and `def getLabel = label` to sort that out. (The other failure is a test-only case class.) At least, those are the only failures that turned up so far here.
I suppose the only catch is that's kind of a behavior change, if case classes stop working, even though they weren't intended to work. Or is it a change -- I don't quite see that they ever fully worked? they just happened to because the tests didn't cause a case class to be used with the serialize/deserialize path?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16022: [SPARK-18598][SQL] Fixed inconsistent bean property infe...
Posted by hamishmorgan <gi...@git.apache.org>.
Github user hamishmorgan commented on the issue:
https://github.com/apache/spark/pull/16022
Okay. If it can't be done I'm happy to close the PR. I'll leave the JIRA ticket open though, since it's still a bug IMO. Thanks for the help guys.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16022: [SPARK-18598][SQL] Fixed inconsistent bean proper...
Posted by hamishmorgan <gi...@git.apache.org>.
Github user hamishmorgan closed the pull request at:
https://github.com/apache/spark/pull/16022
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org