You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by huaxingao <gi...@git.apache.org> on 2016/05/25 00:26:26 UTC
[GitHub] spark pull request: [SPARK-15491][SQL]fix assertion failure for JD...
GitHub user huaxingao opened a pull request:
https://github.com/apache/spark/pull/13287
[SPARK-15491][SQL]fix assertion failure for JDBC DataFrame to JSON
## What changes were proposed in this pull request?
in TreeNode.scala parseToJson, it has
case p: Product => try {
val fieldNames = getConstructorParameterNames(p.getClass)
val fieldValues = p.productIterator.toSeq
assert(fieldNames.length == fieldValues.length)
("product-class" -> JString(p.getClass.getName)) :: fieldNames.zip(fieldValues).map {
case (name, value) => name -> parseToJson(value)
}.toList
For a class that has two level of constructor parameters, for example,
private[sql] case class JDBCRelation(
url: String,
table: String,
parts: Array[Partition],
properties: Properties = new Properties())(@transient val sparkSession: SparkSession)
val fieldValues = p.productIterator.toSeq will only get the values for the first level of constructor parameters, in this case, 4 parameters, but val fieldNames = getConstructorParameterNames(p.getClass) will get all the 5 parameters, so assert(fieldNames.length == fieldValues.length) will fail.
I will change code to make fieldValues have all the parameters.
## How was this patch tested?
I have a unit test. Will add it soon.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/huaxingao/spark spark-15491
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/13287.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 #13287
----
commit 8307b348b62f0653cccd04cd543cf77281bc4b85
Author: Huaxin Gao <hu...@us.ibm.com>
Date: 2016-05-25T00:11:53Z
[SPARK-15491][SQL]fix assertion failure for JDBC DataFrame to JSON
----
---
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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/13287#discussion_r123162595
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala ---
@@ -594,7 +596,9 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
// returns null if the product type doesn't have a primary constructor, e.g. HiveFunctionWrapper
case p: Product => try {
--- End diff --
Could you check the master branch? We limit `toJson` to limited node types after https://github.com/apache/spark/pull/14990. `shouldConvertToJson`. It sounds like the class you mentioned above is not part of the support types
---
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: [SPARK-15491][SQL]fix assertion failure for JD...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/13287#issuecomment-221476178
Thanks for the pull request. Can you format the description better?
---
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: [SPARK-15491][SQL]fix assertion failure for JD...
Posted by huaxingao <gi...@git.apache.org>.
Github user huaxingao commented on the pull request:
https://github.com/apache/spark/pull/13287#issuecomment-221617823
@rxin Sorry, I didn't notice your comment until this morning. Reformatted the description. Thanks!
---
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: [SPARK-15491][SQL]fix assertion failure for JD...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/13287#issuecomment-221441031
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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC DataFra...
Posted by huaxingao <gi...@git.apache.org>.
Github user huaxingao commented on the issue:
https://github.com/apache/spark/pull/13287
@rxin gentle ping
---
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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC ...
Posted by huaxingao <gi...@git.apache.org>.
Github user huaxingao commented on a diff in the pull request:
https://github.com/apache/spark/pull/13287#discussion_r123818099
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala ---
@@ -594,7 +596,9 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
// returns null if the product type doesn't have a primary constructor, e.g. HiveFunctionWrapper
case p: Product => try {
--- End diff --
Thanks. I will close 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 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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC DataFra...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/13287
Is it still an issue?
---
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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC ...
Posted by huaxingao <gi...@git.apache.org>.
Github user huaxingao commented on a diff in the pull request:
https://github.com/apache/spark/pull/13287#discussion_r123368171
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala ---
@@ -594,7 +596,9 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
// returns null if the product type doesn't have a primary constructor, e.g. HiveFunctionWrapper
case p: Product => try {
--- End diff --
Thank you very much for your reply.
Shall we also allow the JDBCRelation to be converted to JSON like the following?
```
val jdbcDF = sqlContext.read.format("jdbc").options(
Map("url" -> "jdbc:h2:mem:testdb0;user=testUser;password=testPass",
"dbtable" -> "people")).load().queryExecution.logical.toJSON
```
---
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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC ...
Posted by huaxingao <gi...@git.apache.org>.
Github user huaxingao closed the pull request at:
https://github.com/apache/spark/pull/13287
---
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: [SPARK-15491][SQL]fix assertion failure for JD...
Posted by huaxingao <gi...@git.apache.org>.
Github user huaxingao commented on the pull request:
https://github.com/apache/spark/pull/13287#issuecomment-222221356
@rxin Thanks for your comment.
When I wrote the test, I found there is one more place I need to change. So there are two places that have problems:
1. TreeNode that has two layers of constructor parameters.
2. TreeNode has a constructor parameter that contains two layers of constructor parameters.
I have both cases in my test. Hopefully it is what you want. Thanks!
---
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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/13287#discussion_r123810649
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala ---
@@ -594,7 +596,9 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
// returns null if the product type doesn't have a primary constructor, e.g. HiveFunctionWrapper
case p: Product => try {
--- End diff --
Maybe not needed now unless it is 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 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: [SPARK-15491][SQL]fix assertion failure for JD...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/13287#issuecomment-221739911
Can we create a unit test for TreeNode, rather than using JDBC?
---
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 #13287: [SPARK-15491][SQL]fix assertion failure for JDBC DataFra...
Posted by huaxingao <gi...@git.apache.org>.
Github user huaxingao commented on the issue:
https://github.com/apache/spark/pull/13287
@gatorsmile
Sorry for the late response. I just came back from China.
For a class that has two level of constructor parameters, e.g.
private[sql] case class JDBCRelation(
url: String,
table: String,
parts: Array[Partition],
properties: Properties = new Properties())(@transient val sparkSession: SparkSession)
toJson will have problem. The current code doesn't have problem any more because in the new method TreeNode.shouldConvertToJson, JDBCRelation returns false and is not allowed to convert to JSON. However, it seems to me that the root problem is not fixed. In the following cases:
1. TreeNode that has two layers of constructor parameters.
2. TreeNode has a constructor parameter that contains two layers of constructor parameters.
And if the TreeNode can be converted to JSON, the Exception will still occur.
---
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