Skip to content

Conversation

@chihhanyu
Copy link
Contributor

What changes were proposed in this pull request?

JIRA issue: https://issues.apache.org/jira/browse/SPARK-21658

Add default None for value in na.replace since Dataframe.replace and DataframeNaFunctions.replace are alias.

The default values are the same now.

>>> df = sqlContext.createDataFrame([('Alice', 10, 80.0)])
>>> df.replace({"Alice": "a"}).first()
Row(_1=u'a', _2=10, _3=80.0)
>>> df.na.replace({"Alice": "a"}).first()
Row(_1=u'a', _2=10, _3=80.0)

How was this patch tested?

Existing tests.

cc @viirya

@HyukjinKwon
Copy link
Member

Could we add the example in the doctest (under 1362L) so that this can be tested and shown in the documentation?

@HyukjinKwon
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Aug 10, 2017

Test build #80469 has finished for PR 18895 at commit 8af1e15.

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

@viirya
Copy link
Member

viirya commented Aug 10, 2017

@byakuinss Please add a doc test in DataFrame.replace. There is an example df4.na.replace('Alice', None).show(). We want to make sure it works with default value. Thanks.

@SparkQA
Copy link

SparkQA commented Aug 10, 2017

Test build #80490 has finished for PR 18895 at commit d72c231.

  • This patch fails Python style tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2017

Test build #80530 has finished for PR 18895 at commit 5d8d4b6.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

I think it is close to get merged. Would you address the comments @byakuinss ?

Copy link
Member

Choose a reason for hiding this comment

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

looks trailing white spaces should be removed. Could we remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your reminding! I'll remove them.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is okay to leave this line as was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I'll change them back.

@SparkQA
Copy link

SparkQA commented Aug 14, 2017

Test build #80632 has finished for PR 18895 at commit abdef40.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member

viirya commented Aug 14, 2017

LGTM

@SparkQA
Copy link

SparkQA commented Aug 14, 2017

Test build #80633 has finished for PR 18895 at commit d07d49a.

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

@HyukjinKwon
Copy link
Member

LGTM too.

@HyukjinKwon
Copy link
Member

Merged to master.

@HyukjinKwon
Copy link
Member

@byakuinss, BTW, do you mind if I ask your JIRA id? I want to assign this to you as you resolved this but I can't find the ID..

@asfgit asfgit closed this in 0fcde87 Aug 14, 2017
@chihhanyu
Copy link
Contributor Author

@HyukjinKwon
Oh, do you mean my jira full name? It's Chin Han Yu.

@HyukjinKwon
Copy link
Member

Hm.. weird. I can't search your account on JIRA ...

@HyukjinKwon
Copy link
Member

Can you maybe leave any comment saying.. like .. "here is my JIRA account." in https://issues.apache.org/jira/browse/SPARK-21658 if you don't mind?

@chihhanyu
Copy link
Contributor Author

Okay, I leave a comment in the issue page.

@viirya
Copy link
Member

viirya commented Aug 14, 2017

Thanks! @HyukjinKwon

@HyukjinKwon
Copy link
Member

Guys, I am sorry but I think I have to revert this by few explicit objections, for now.
Maybe, I am now rushing to revert it but please don't mind because we are close to 2.3.0.

Please join in the discussion about the original implementation details in #16793.

Thanks.

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.

4 participants