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.

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

fixed parsing of Timestamp and TimestampTz columns in Python3 #119

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dennisobrien
Contributor

dennisobrien commented May 16, 2016

This modifies timestamp_parse and timestamp_tz_parse to coerce the argument to a utf-8 encode string. In Python2 the argument is already a string while in Python3 it is a bytes object.

These three queries threw exceptions that are fixed by this PR:

connection = vertica_python.connect(**config)
cursor = connection.cursor()

# calls timestamp_parse
query = """
select
  to_timestamp('2016-05-15 13:15:17.789', 'YYYY-MM-DD HH:MI:SS.MS')
;
"""
cursor.execute(query)
cursor.fetchall()

# calls timestamp_tz_parse
query = """
select
  to_timestamp_tz('2016-05-15 13:15:17.789 UTC', 'YYYY-MM-DD HH:MI:SS.MS TZ')
;
"""
cursor.execute(query)
cursor.fetchall()

# calls timestamp_tz_parse
query = """
select
  timestamp '2016-05-15 13:15:17.789+00'
;
"""
cursor.execute(query)
cursor.fetchall()

It may be that we should be coercing to unicode somewhere before the parse methods are called. Let me know if that is the case and I can move it sooner in the pipeline.

thanks,
Dennis

@dennisobrien

This comment has been minimized.

Contributor

dennisobrien commented May 16, 2016

BTW, this addresses #117

@jbfavre

This comment has been minimized.

jbfavre commented May 22, 2016

I just tried your PR against 0.6.2 release, and it breaks tests:

> python3 --version
Python 3.5.1+
> python3 ~/.local/bin/nosetests
...................EEE...F.
======================================================================
ERROR: test_timestamp_parser (vertica_python.tests.date_tests.TimestampParsingTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/Debian/python-modules/python-vertica/vertica_python/tests/date_tests.py", line 59, in test_timestamp_parser
    parsed_timestamp = timestamp_parse('1841-05-05 22:07:58')
  File "/home/dev/Debian/python-modules/python-vertica/vertica_python/vertica/column.py", line 37, in timestamp_parse
    s = str(s, 'utf-8')
TypeError: decoding str is not supported

======================================================================
ERROR: test_timestamp_with_year_over_9999 (vertica_python.tests.date_tests.TimestampParsingTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/Debian/python-modules/python-vertica/vertica_python/tests/date_tests.py", line 64, in test_timestamp_with_year_over_9999
    parsed_timestamp = timestamp_parse('44841-05-05 22:07:58')
  File "/home/dev/Debian/python-modules/python-vertica/vertica_python/vertica/column.py", line 37, in timestamp_parse
    s = str(s, 'utf-8')
TypeError: decoding str is not supported

======================================================================
ERROR: test_timestamp_with_year_over_9999_and_ms (vertica_python.tests.date_tests.TimestampParsingTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/Debian/python-modules/python-vertica/vertica_python/tests/date_tests.py", line 69, in test_timestamp_with_year_over_9999_and_ms
    parsed_timestamp = timestamp_parse('124841-05-05 22:07:58.000003')
  File "/home/dev/Debian/python-modules/python-vertica/vertica_python/vertica/column.py", line 37, in timestamp_parse
    s = str(s, 'utf-8')
TypeError: decoding str is not supported```
@jbfavre

This comment has been minimized.

jbfavre commented May 22, 2016

My guess is that because tests call timestamp_parse function with a string parameter, when it should be bytes.
Tests pass with following patch:

diff --git a/vertica_python/tests/date_tests.py b/vertica_python/tests/date_tests.py
index 4477165..f7c797a 100644
--- a/vertica_python/tests/date_tests.py
+++ b/vertica_python/tests/date_tests.py
@@ -56,16 +56,19 @@ class TimestampParsingTestCase(VerticaTestCase):


     def test_timestamp_parser(self):
-        parsed_timestamp = timestamp_parse('1841-05-05 22:07:58')
+        test_timestamp = '1841-05-05 22:07:58'.encode(encoding='utf-8', errors='strict')
+        parsed_timestamp = timestamp_parse(test_timestamp)
         # Assert parser default to strptime
         self.assertEqual(datetime(year=1841, month=5, day=5, hour=22, minute=7, second=58), parsed_timestamp)

     def test_timestamp_with_year_over_9999(self):
-        parsed_timestamp = timestamp_parse('44841-05-05 22:07:58')
+        test_timestamp = '44841-05-05 22:07:58'.encode(encoding='utf-8', errors='strict')
+        parsed_timestamp = timestamp_parse(test_timestamp)
         # Assert year was truncated properly
         self.assertEqual(datetime(year=4841, month=5, day=5, hour=22, minute=7, second=58), parsed_timestamp)

     def test_timestamp_with_year_over_9999_and_ms(self):
-        parsed_timestamp = timestamp_parse('124841-05-05 22:07:58.000003')
+        test_timestamp = '124841-05-05 22:07:58.000003'.encode(encoding='utf-8', errors='strict')
+        parsed_timestamp = timestamp_parse(test_timestamp)
         # Assert year was truncated properly
         self.assertEqual(datetime(year=4841, month=5, day=5, hour=22, minute=7, second=58, microsecond=3), parsed_timestamp)```
@alexjikim

This comment has been minimized.

Contributor

alexjikim commented May 23, 2016

I think this + jbfavre's patch + one other change fixes the problem. commit is here:
e13f08e

@alexjikim alexjikim closed this May 23, 2016

@dennisobrien

This comment has been minimized.

Contributor

dennisobrien commented May 28, 2016

Thanks @alexjikim and @jbfavre for sorting this out.

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