Skip to content
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

Update PostgreSQL driver to latest #5712

Closed
wants to merge 0 commits into from

Conversation

adamjshook
Copy link
Member

Updates presto-root to use the latest postgresql driver to address bug fixes, specifically time type handling in pgjdbc/pgjdbc#1570.

checker-qual exclusions are to address an enforcer failure; see below for example. Let me know if this should be addressed a different way.

+-io.prestosql:presto-postgresql:346-SNAPSHOT
  +-com.google.guava:guava:29.0-jre
    +-org.checkerframework:checker-qual:2.11.1
and
+-io.prestosql:presto-postgresql:346-SNAPSHOT
  +-org.postgresql:postgresql:42.2.16
    +-org.checkerframework:checker-qual:3.5.0

@cla-bot cla-bot bot added the cla-signed label Oct 27, 2020
@findepi
Copy link
Member

findepi commented Oct 27, 2020

Updates presto-root to use the latest postgresql driver to address bug fixes, specifically time type handling in pgjdbc/pgjdbc#1570.

should io.prestosql.plugin.postgresql.PostgreSqlClient#timeColumnMapping be updated?
also, is there any known user visible change from the upgrade?

@adamjshook
Copy link
Member Author

I'll see if I can write a test case to expose this issue. The issue is with the toLocalTimeBin which may not be hit.

@findepi
Copy link
Member

findepi commented Oct 27, 2020

I'll see if I can write a test case to expose this issue.

You can see existing tests io.prestosql.plugin.postgresql.TestPostgreSqlTypeMapping#testTimeCoercion and io.prestosql.plugin.postgresql.TestPostgreSqlTypeMapping#testTime as a starting point.

@adamjshook
Copy link
Member Author

@findepi I am able to reproduce this by setting prepareThreshold=-1 in the JDBC URL for the testing server to force binary transmission, then run testTime24, which results in the error Invalid value for NanoOfDay (valid values 0 - 86399999999999): 86400000000000.

Interestingly enough, updating the driver to the latest version doesn't seem to help and it still fails with that error. Looking at using result.getString and parsing with ISO_LOCAL_TIME seems to lose some precision in the time type, and return wrong values? If binary types are not forced, result.getString works okay. Seems like binary types are all kinds of broken. Maybe this isn't really too much of a concern as it requires users to force transmission of binary time types.

java.lang.AssertionError: [Rows] 
Expecting:
  <(01:00, 01:00, 01:00), (23:59:59, 23:59:59.999, 23:59:59.999), (01:00, 01:00, 01:00)>
to contain exactly in any order:
  <[(00:00, 00:00, 00:00),
    (23:59:59, 23:59:59.999, 23:59:59.999999),
    (23:59:59, 23:59:59.999, 23:59:59.999999)]>
elements not found:
  <(00:00, 00:00, 00:00), (23:59:59, 23:59:59.999, 23:59:59.999999), (23:59:59, 23:59:59.999, 23:59:59.999999)>
and elements not expected:
  <(01:00, 01:00, 01:00), (23:59:59, 23:59:59.999, 23:59:59.999), (01:00, 01:00, 01:00)>

@findepi
Copy link
Member

findepi commented Oct 29, 2020

I am able to reproduce this by setting prepareThreshold=-1 in the JDBC URL for the testing server to force binary transmission, then run testTime24, which results in the error Invalid value for NanoOfDay (valid values 0 - 86399999999999): 86400000000000.

@adamjshook what is the meaning of the prepareThreshold parameter?
am i reading this correctly that users will run into problems if they run similar/same queries over Presto?
and we are just "lucky" in testing because we issue too few queries?

@adamjshook
Copy link
Member Author

I was looking for a means to force the JDBC client to retrieve the time in a binary format in order to have it use toLocalTimeBin to trigger this bug. I came across this JDBC property in order to have it force binary types -- not sure what it is actually meant to do.

prepareThreshold: Statement prepare threshold. A value of -1 stands for forceBinary

I think we are lucky in the sense that this function isn't used in any of the tests, but I don't have enough information to know under what circumstances a user would hit this problem -- any time the 24:00:00 time value is read in a binary format it would fail.

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

Successfully merging this pull request may close these issues.

2 participants