Skip to content

[SPARK-10847] [SQL] [PySpark] Pyspark - DataFrame - Optional Metadata with None triggers cryptic failure - #8969

Closed
jasoncl wants to merge 5 commits into
apache:masterfrom
jasoncl:SPARK-10847
Closed

[SPARK-10847] [SQL] [PySpark] Pyspark - DataFrame - Optional Metadata with None triggers cryptic failure#8969
jasoncl wants to merge 5 commits into
apache:masterfrom
jasoncl:SPARK-10847

Conversation

@jasoncl

@jasoncljasoncl commented Oct 2, 2015

Copy link
Copy Markdown
Contributor

The error message is now changed from "Do not support type class scala.Tuple2." to "Do not support type class org.json4s.JsonAST$JNull$" to be more informative about what is not supported. Also, StructType metadata now handles JNull correctly, i.e., {'a': None}. test_metadata_null is added to tests.py to show the fix works.

@shea-parkes

shea-parkes commented Oct 3, 2015

Copy link
Copy Markdown
Contributor

Thanks for doing this!

@JoshRosen

JoshRosen commented Oct 18, 2015

Copy link
Copy Markdown
Contributor

Jenkins, this is ok to test.

JoshRosen
JoshRosen reviewed Oct 18, 2015
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/types/Metadata.scala Outdated

@JoshRosenJoshRosenOct 18, 2015

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.

This comment is incorrect. Also, please leave an additional line of whitespace before this line.

@JoshRosen

JoshRosen commented Oct 18, 2015

Copy link
Copy Markdown
Contributor

This seems good overall, but one high-level question: why store None on the Java side instead of null? I'm just wondering whether mapping it back to Java null would make more sense. I guess it doesn't make a difference from Python's point of view, since Python's view of the metadata is preserved roundtrip, but it could hypothetically matter if Python is trying to set metadata which is then consumed by Java library code (for example).

@SparkQA

SparkQA commented Oct 18, 2015

Copy link
Copy Markdown

Test build #43900 has finished for PR 8969 at commit 6bb4fcc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Oct 23, 2015

Copy link
Copy Markdown

Test build #44246 has finished for PR 8969 at commit 5524a92.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Oct 23, 2015

Copy link
Copy Markdown

Test build #44243 has finished for PR 8969 at commit 978fdc7.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@jasoncl

jasoncl commented Oct 23, 2015

Copy link
Copy Markdown
ContributorAuthor

The test build #44243 for commit 978fdc7 shows that the patch does not merge cleanly. But that is not my latest commit. My latest commit 5524a92 has passed the test build and will merge cleanly.

yhuai
yhuai reviewed Jan 13, 2016
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/types/Metadata.scala

@yhuaiyhuaiJan 13, 2016

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.

Looks like not worth adding a method since it is just used once?

@jasoncljasonclJan 14, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The method is created for code organization and clarity purpose. This way it can be easily reused in the future.

@yhuai

yhuai commented Jan 13, 2016

Copy link
Copy Markdown
Contributor

@jasoncl Can you do a quick update if my comment makes sense? Then, we will get it merged. Thanks!

@JoshRosen

JoshRosen commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

Ping @yhuai, is this ready for merging?

@JoshRosen

JoshRosen commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

Jenkins, retest this please.

@SparkQA

SparkQA commented Jan 27, 2016

Copy link
Copy Markdown

Test build #50160 has finished for PR 8969 at commit 5524a92.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yhuai

yhuai commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

LGTM. I am merging it.

asfgit pushed a commit that referenced this pull request Jan 27, 2016
@jasoncl@yhuai
[SPARK-10847][SQL][PYSPARK] Pyspark - DataFrame - Optional Metadata w…
96e32db
…ith `None` triggers cryptic failure
The error message is now changed from "Do not support type class scala.Tuple2." to "Do not support type class org.json4s.JsonAST$JNull$" to be more informative about what is not supported. Also, StructType metadata now handles JNull correctly, i.e., {'a': None}. test_metadata_null is added to tests.py to show the fix works.
Author: Jason Lee <cjlee@us.ibm.com>
Closes#8969 from jasoncl/SPARK-10847.
(cherry picked from commit edd4737)
Signed-off-by: Yin Huai <yhuai@databricks.com>
asfgit pushed a commit that referenced this pull request Jan 27, 2016
@jasoncl@yhuai
[SPARK-10847][SQL][PYSPARK] Pyspark - DataFrame - Optional Metadata w…
49dc8e7
…ith `None` triggers cryptic failure
The error message is now changed from "Do not support type class scala.Tuple2." to "Do not support type class org.json4s.JsonAST$JNull$" to be more informative about what is not supported. Also, StructType metadata now handles JNull correctly, i.e., {'a': None}. test_metadata_null is added to tests.py to show the fix works.
Author: Jason Lee <cjlee@us.ibm.com>
Closes#8969 from jasoncl/SPARK-10847.
(cherry picked from commit edd4737)
Signed-off-by: Yin Huai <yhuai@databricks.com>
@yhuai

yhuai commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

I also merged it in branch 1.5 and branch 1.6 since it is very isolated bug fix.

@shea-parkes

shea-parkes commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

Thanks a bunch guys, this will be a big help.

@gatorsmilegatorsmile mentioned this pull request Jul 9, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jasoncl@shea-parkes@JoshRosen@SparkQA@yhuai