[SPARK-28003][PYTHON] Allow NaT values when creating Spark dataframe from pandas with Arrow #24844

icexelloss · 2019-06-11T20:31:22Z

What changes were proposed in this pull request?

This patch removes fillna(0) when creating ArrowBatch from a pandas Series.

With fillna(0), the original code would turn a timestamp type into object type, which pyarrow will complain later:

>>> s = pd.Series([pd.NaT, pd.Timestamp('2015-01-01')])
>>> s.dtypes
dtype('<M8[ns]')
>>> s.fillna(0)
0                      0
1    2015-01-01 00:00:00
dtype: object

How was this patch tested?

Added test_timestamp_nat

icexelloss · 2019-06-11T20:31:43Z

cc @BryanCutler @HyukjinKwon

icexelloss · 2019-06-11T20:33:36Z

python/pyspark/serializers.py

@@ -296,7 +296,7 @@ def create_array(s, t):
            mask = s.isnull()
            # Ensure timestamp series are in expected form for Spark internal representation
            if t is not None and pa.types.is_timestamp(t):
-                s = _check_series_convert_timestamps_internal(s.fillna(0), self._timezone)
+                s = _check_series_convert_timestamps_internal(s, self._timezone)


Removing this doesn't fail existing tests. @BryanCutler do you remember why are we doing fillna(0) here?

I believe it was due to a Pandas error, most likely because we were testing with 0.19.2 at the time. Can you manually run some tests with different Pandas versions? It will be best to test with older versions, but it might be kind of hard to get 0.19.2 working with pyarrow 0.12.1 though..

Yeah pandas 0.19.2 doesn't work with pyarrow 0.12. I cannot run arrow tests with pandas 0.19.2 anymore.

Since we are requiring min arrow version to be 0.12, it means pandas version 0.19.2 is not supported if the user wants to use Arrow.

SparkQA · 2019-06-11T21:03:31Z

Test build #106395 has finished for PR 24844 at commit 8bfefed.

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

BryanCutler

I think this is probably safe to remove for newer Pandas versions, but we will want to check some older versions as well

BryanCutler · 2019-06-12T17:34:06Z

python/pyspark/serializers.py

@@ -296,7 +296,7 @@ def create_array(s, t):
            mask = s.isnull()
            # Ensure timestamp series are in expected form for Spark internal representation
            if t is not None and pa.types.is_timestamp(t):
-                s = _check_series_convert_timestamps_internal(s.fillna(0), self._timezone)
+                s = _check_series_convert_timestamps_internal(s, self._timezone)


I believe it was due to a Pandas error, most likely because we were testing with 0.19.2 at the time. Can you manually run some tests with different Pandas versions? It will be best to test with older versions, but it might be kind of hard to get 0.19.2 working with pyarrow 0.12.1 though..

BryanCutler · 2019-06-12T17:41:34Z

I think we are testing with Pandas 0.23.2 in the Jenkins env. Maybe it is a good time to bump up the minimum Pandas supported since 0.19.2 is pretty ancient. Thoughts @HyukjinKwon @felixcheung @shaneknapp @ueshin ?

BryanCutler · 2019-06-12T17:44:21Z

python/pyspark/sql/tests/test_arrow.py

@@ -383,6 +383,19 @@ def test_timestamp_dst(self):
        assert_frame_equal(pdf, df_from_python.toPandas())
        assert_frame_equal(pdf, df_from_pandas.toPandas())

+    def test_timestamp_nat(self):
+        import pandas as pd


This import is already at the top

BryanCutler · 2019-06-12T17:45:10Z

python/pyspark/sql/tests/test_arrow.py

+        pdf2 = pd.DataFrame({'time': dt2})
+
+        df1 = self.spark.createDataFrame(pdf1)
+        df2 = self.spark.createDataFrame(pdf2)


I think you can just combine these to 1 DataFrame, but I think it would be good to also check against toPandas without Arrow

Also checked non-arrow codepath

SparkQA · 2019-06-12T22:54:08Z

Test build #106446 has finished for PR 24844 at commit 4af2aed.

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

HyukjinKwon · 2019-06-13T07:08:40Z

Yea, +1 for bumping up Pandas. 0.19.2 is pretty old and I think it's good time to bump up in Spark 3.

HyukjinKwon · 2019-06-13T07:10:28Z

python/pyspark/sql/tests/test_arrow.py

+        dt = [pd.NaT, pd.Timestamp('2019-06-11'), None] * 100
+        pdf = pd.DataFrame({'time': dt})
+
+        with self.sql_conf({'spark.sql.execution.arrow.pyspark.enabled': "false"}):


not a big deal but I think we can do this with a for loop.

Good point refactored to forloop

icexelloss · 2019-06-13T15:49:01Z

+1 for bumping up pandas. I could open a Jira for bumping that. Is there other things we'd like to address in this PR?

viirya · 2019-06-13T15:59:26Z

python/pyspark/serializers.py

@@ -296,7 +296,7 @@ def create_array(s, t):
            mask = s.isnull()
            # Ensure timestamp series are in expected form for Spark internal representation
            if t is not None and pa.types.is_timestamp(t):
-                s = _check_series_convert_timestamps_internal(s.fillna(0), self._timezone)
+                s = _check_series_convert_timestamps_internal(s, self._timezone)
                # TODO: need cast after Arrow conversion, ns values cause error with pandas 0.19.2


This looks related to pandas 0.19.2 too. Should we deal with it? If we want to bump up pandas.

Let's fix this one up in another PR.

I fixed this here as well since it's pretty small. I am happy to revert and make it a seperate PR if we prefer that. Please let me know :)

SparkQA · 2019-06-13T16:21:42Z

Test build #106475 has finished for PR 24844 at commit 7b1d964.

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

BryanCutler · 2019-06-13T17:19:04Z

Let's change the minimum Pandas version in another PR. I made https://issues.apache.org/jira/browse/SPARK-28041. I think we should merge this one after the version is increased.

BryanCutler · 2019-06-13T17:24:05Z

python/pyspark/sql/tests/test_arrow.py

+        dt = [pd.NaT, pd.Timestamp('2019-06-11'), None] * 100
+        pdf = pd.DataFrame({'time': dt})
+
+        for arrow_enabled in [False, True]:


you can just use _toPandas_arrow_toggle which returns 2 DataFrames with and without arrow

Ah thanks for the tip! (I ended up using _createDataFrame_toggle because that's the path I want to test)

SparkQA · 2019-06-13T19:05:00Z

Test build #106480 has finished for PR 24844 at commit d479c41.

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

SparkQA · 2019-06-13T19:08:54Z

Test build #106481 has finished for PR 24844 at commit 6be6d77.

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

SparkQA · 2019-06-13T19:30:49Z

Test build #106482 has finished for PR 24844 at commit 594e40b.

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

shaneknapp · 2019-06-14T17:25:07Z

as mentioned on dev@ and other github issues, we're testing against 0.24.2 and will continue to do so.

…

On Wed, Jun 12, 2019 at 10:41 AM Bryan Cutler ***@***.***> wrote: I think we are testing with Pandas 0.23.2 in the Jenkins env. Maybe it is a good time to bump up the minimum Pandas supported since 0.19.2 is pretty ancient. Thoughts @HyukjinKwon <https://github.com/HyukjinKwon> @felixcheung <https://github.com/felixcheung> @shaneknapp <https://github.com/shaneknapp> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24844?email_source=notifications&email_token=AAMIHLHOGCGJX6RQRIEAASTP2EYOJA5CNFSM4HXDDMBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXRHQYQ#issuecomment-501381218>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMIHLCV7HKT4EXCDDJVGS3P2EYOJANCNFSM4HXDDMBA> .

icexelloss · 2019-06-17T19:46:57Z

Ping. Anything else we want for this PR?

HyukjinKwon · 2019-06-18T00:11:58Z

K, minimal version is updated now as of #24867

BryanCutler · 2019-06-18T16:06:51Z

@icexelloss could you resolve conflicts and manually test with Pandas 0.23.2?

SparkQA · 2019-06-21T19:26:40Z

Test build #106777 has finished for PR 24844 at commit bda31b5.

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

icexelloss · 2019-06-24T17:38:04Z

Manually tested all arrow related tests with 0.23.2

BryanCutler

LGTM

BryanCutler · 2019-06-24T18:16:14Z

merged to master, thanks @icexelloss !

icexelloss · 2019-06-24T20:23:57Z

Thanks!

HyukjinKwon

LGTM too!

…from pandas with Arrow ## What changes were proposed in this pull request? This patch removes `fillna(0)` when creating ArrowBatch from a pandas Series. With `fillna(0)`, the original code would turn a timestamp type into object type, which pyarrow will complain later: ``` >>> s = pd.Series([pd.NaT, pd.Timestamp('2015-01-01')]) >>> s.dtypes dtype('<M8[ns]') >>> s.fillna(0) 0 0 1 2015-01-01 00:00:00 dtype: object ``` ## How was this patch tested? Added `test_timestamp_nat` Closes apache#24844 from icexelloss/SPARK-28003-arrow-nat. Authored-by: Li Jin <ice.xelloss@gmail.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

icexelloss commented Jun 11, 2019

View reviewed changes

BryanCutler reviewed Jun 12, 2019

View reviewed changes

dongjoon-hyun added the IMPROVEMENT label Jun 12, 2019

HyukjinKwon reviewed Jun 13, 2019

View reviewed changes

viirya reviewed Jun 13, 2019

View reviewed changes

BryanCutler reviewed Jun 13, 2019

View reviewed changes

icexelloss force-pushed the SPARK-28003-arrow-nat branch from 6be6d77 to 594e40b Compare June 13, 2019 18:50

dongjoon-hyun added PYSPARK and removed IMPROVEMENT labels Jun 14, 2019

icexelloss added 4 commits June 21, 2019 11:40

Fix NaT bug with Arrow

af6c2db

Simplify test cases; Test both arrow and non arrow path

1c5b8a7

Add Jira numbers

ae144e1

Refactor to forloop

60733ac

Use _createDataFrame_toggle

bda31b5

icexelloss force-pushed the SPARK-28003-arrow-nat branch from 594e40b to bda31b5 Compare June 21, 2019 18:47

BryanCutler approved these changes Jun 24, 2019

View reviewed changes

BryanCutler closed this in d0fbc4d Jun 24, 2019

HyukjinKwon reviewed Jun 25, 2019

View reviewed changes

rshkv mentioned this pull request May 23, 2020

Misc PyArrow fixes palantir/spark#684

Merged

9 tasks

[SPARK-28003][PYTHON] Allow NaT values when creating Spark dataframe from pandas with Arrow #24844

[SPARK-28003][PYTHON] Allow NaT values when creating Spark dataframe from pandas with Arrow #24844

Conversation

icexelloss commented Jun 11, 2019

What changes were proposed in this pull request?

How was this patch tested?

icexelloss commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jun 11, 2019

BryanCutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler commented Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 12, 2019

HyukjinKwon commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss commented Jun 13, 2019

viirya Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 13, 2019

BryanCutler commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 13, 2019

SparkQA commented Jun 13, 2019

SparkQA commented Jun 13, 2019

shaneknapp commented Jun 14, 2019 via email

icexelloss commented Jun 17, 2019

HyukjinKwon commented Jun 18, 2019

BryanCutler commented Jun 18, 2019

SparkQA commented Jun 21, 2019

icexelloss commented Jun 24, 2019

BryanCutler left a comment

Choose a reason for hiding this comment

BryanCutler commented Jun 24, 2019

icexelloss commented Jun 24, 2019

HyukjinKwon left a comment

Choose a reason for hiding this comment

icexelloss Jun 12, 2019 •

edited

Loading

BryanCutler commented Jun 12, 2019 •

edited

Loading

viirya Jun 13, 2019 •

edited

Loading