Skip to content

Conversation

@sparkyengine
Copy link
Contributor

java mapwithstate with Function3 has wrong conversion of java Optional to scala Option, fixed code uses same conversion used in the mapwithstate call that uses Function4 as an input. Optional.fromNullable(v.get) fails if v is None, better to use JavaUtils.optionToOptional(v) instead.

java mapwithstate with Function3 has wrong conversion of java Optional to scala Option
@holdenk
Copy link
Contributor

holdenk commented Feb 1, 2016

Hi @gabrielenizzoli - generally Spark require's that there is a JIRA associated the proposed change as well as having it mentioned in the pull request title. This is also just merging against the 1.6 branch but generally most changes are applied to master and then explicitly back ported (looking in master it seems to have changed as part of the migration away from having guava in the public API to Optional.ofNullable). The guide at https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark can be quite useful for structuring your pull request.

@zsxwing
Copy link
Member

zsxwing commented Feb 1, 2016

Jenkins, test this please

@zsxwing
Copy link
Member

zsxwing commented Feb 1, 2016

@gabrielenizzoli Thanks, good catch. Please follow the instructions in https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark

@SparkQA
Copy link

SparkQA commented Feb 1, 2016

Test build #50501 has finished for PR 11007 at commit 78c0323.

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

@sparkyengine sparkyengine changed the title java mapwithstate, broken java mapping [SPARK-13121] [Streaming] java mapWithState mishandles scala Option Feb 1, 2016
@zsxwing
Copy link
Member

zsxwing commented Feb 2, 2016

retest this please

@SparkQA
Copy link

SparkQA commented Feb 2, 2016

Test build #50516 has finished for PR 11007 at commit 78c0323.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member

zsxwing commented Feb 2, 2016

retest this please

@SparkQA
Copy link

SparkQA commented Feb 2, 2016

Test build #50544 has finished for PR 11007 at commit 78c0323.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Feb 2, 2016

This sounds correct to me.

@zsxwing
Copy link
Member

zsxwing commented Feb 2, 2016

retest this please

@SparkQA
Copy link

SparkQA commented Feb 2, 2016

Test build #50571 has finished for PR 11007 at commit 78c0323.

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

@zsxwing
Copy link
Member

zsxwing commented Feb 2, 2016

LGTM. Merging to master and 1.6. Thanks @gabrielenizzoli

asfgit pushed a commit that referenced this pull request Feb 2, 2016
java mapwithstate with Function3 has wrong conversion of java `Optional` to scala `Option`, fixed code uses same conversion used in the mapwithstate call that uses Function4 as an input. `Optional.fromNullable(v.get)` fails if v is `None`, better to use `JavaUtils.optionToOptional(v)` instead.

Author: Gabriele Nizzoli <mail@nizzoli.net>

Closes #11007 from gabrielenizzoli/branch-1.6.
@zsxwing
Copy link
Member

zsxwing commented Feb 2, 2016

@gabrielenizzoli I have merged this PR to branch 1.6. Could you open a new PR to fix the master branch and close this one? Thanks!

@sparkyengine
Copy link
Contributor Author

@zsxwing yes, I will close this PR and create a new one for master. Should I assign the same jira issue number to the PR for master?

@zsxwing
Copy link
Member

zsxwing commented Feb 2, 2016

@zsxwing yes, I will close this PR and create a new one for master. Should I assign the same jira issue number to the PR for master?

Just make the new PR has the same title of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants