Skip to content

[SPARK-19561][SQL] add int case handling for TimestampType #17200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JasonMWhite wants to merge 5 commits into from
Closed

[SPARK-19561][SQL] add int case handling for TimestampType #17200

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4238533
cast TimestampType.toInternal output to long
Feb 11, 2017
5b1dd67
replace int function call with long
Feb 16, 2017
a1936af
add TimestampType int handling in EvaluatePython, remove casting to l…
Mar 9, 2017
a198d49
linter
Mar 9, 2017
bee635a
add comment explaining adding Int case handling
Mar 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions python/pyspark/sql/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,6 +1435,12 @@ def test_time_with_timezone(self):
self.assertEqual(now, now1)
self.assertEqual(now, utcnow1)

# regression test for SPARK-19561
def test_datetime_at_epoch(self):
epoch = datetime.datetime.fromtimestamp(0)
df = self.spark.createDataFrame([Row(date=epoch)])
self.assertEqual(df.first()['date'], epoch)
Copy link
Member

@viirya viirya Mar 8, 2017

Choose a reason for hiding this comment

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

This test is invalid. df.first()['date'] is None even in current master branch.

Copy link
Author

@JasonMWhite JasonMWhite Mar 8, 2017

Choose a reason for hiding this comment

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

Yes, that's the bug this PR is fixing. It shouldn't be None.


def test_decimal(self):
from decimal import Decimal
schema = StructType([StructField("decimal", DecimalType(10, 5))])
Expand Down
2 changes: 1 addition & 1 deletion python/pyspark/sql/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ def toInternal(self, dt):
if dt is not None:
seconds = (calendar.timegm(dt.utctimetuple()) if dt.tzinfo
Copy link
Member

@viirya viirya Mar 8, 2017

Choose a reason for hiding this comment

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

hmm, for the value of epoch = datetime.datetime.fromtimestamp(0), seconds is 0. What is it different to use int or long?

Copy link
Author

@JasonMWhite JasonMWhite Mar 8, 2017

Choose a reason for hiding this comment

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

The JIRA ticket has the details: https://issues.apache.org/jira/browse/SPARK-19561. But in a nutshell, that's the point: int(0) fails but long(0) succeeds.

Copy link
Author

@JasonMWhite JasonMWhite Mar 8, 2017

Choose a reason for hiding this comment

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

https://github.com/bartdag/py4j/blob/master/py4j-python/src/py4j/protocol.py#L271-L275

Py4J automatically serializes any Python integer larger than 2 ^ 31 as LONG_TYPE, otherwise it's INTEGER_TYPE. Python longs are always serialized as LONG_TYPE.

I suspect my issue with Python 3 is that there is no more long, it's all just int. This may require a fix on the Scala side to accept either an int or a long to the appropriate constructor.

Copy link
Member

@viirya viirya Mar 8, 2017

Choose a reason for hiding this comment

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

Thanks. Interesting.

Copy link
Member

@viirya viirya Mar 8, 2017

Choose a reason for hiding this comment

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

I just tested it. In Python3, even toInternal returns Python's long, you still can a java.lang.Integer in JVM side.

However, in Python2, you can get java.lang.Long.

Copy link
Member

@viirya viirya Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Because Python3 doesn't have long anymore, I think we can't solve this in python. We need to fix this in JVM side.

Copy link
Author

@JasonMWhite JasonMWhite Mar 8, 2017

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member

@viirya viirya Mar 9, 2017

Choose a reason for hiding this comment

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

@JasonMWhite Are you going to submit another PR for it?

Copy link
Author

@JasonMWhite JasonMWhite Mar 9, 2017

Choose a reason for hiding this comment

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

I will, yes. Trying to find where the appropriate location is in the Scala code.

else time.mktime(dt.timetuple()))
return int(seconds) * 1000000 + dt.microsecond
return long(seconds) * 1000000 + dt.microsecond

def fromInternal(self, ts):
if ts is not None:
Expand Down