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

JdbcSourceOperations method setJsonField misses a branch for TIMESTAMP_WITH_TIMEZONE #16838

Closed
tuliren opened this issue Sep 16, 2022 · 2 comments · Fixed by #19860
Closed

JdbcSourceOperations method setJsonField misses a branch for TIMESTAMP_WITH_TIMEZONE #16838

tuliren opened this issue Sep 16, 2022 · 2 comments · Fixed by #19860

Comments

@tuliren
Copy link
Contributor

tuliren commented Sep 16, 2022

Current Behavior

  • Currently TIMESTAMP_WITH_TIMEZONE is not handled by JdbcSourceOperations#setJsonField. This time falls under the default branch, and the output is not the same as what we want (e.g. it does not have microsecond precision).
  • This issue is discovered in Update JDBC driver for Snowflake source #16552 and 🎉 Source Snowflake : Update JDBC driver for Snowflake to 3.13.22 #16766.
    • When the Snowflake JDBC version is bumped from 3.13.9 to 3.13.22, the timestamp with timezone column type from JDBC changed from timestamp to timestamp with timezone. The hypothesis is that the old version of the Snowflake JDBC is not returning the timestamp column type correctly. But it is unclear which version fixed this behavior according to the Snowflake JDBC changelog.
    • Because JdbcSourceOperations is processing timestamp with timezone with the default branch, the output changed from 2018-03-22T07:00:00.123000Z to 2018-03-22 12:00:00.123 +0500.
    • The fix in 🎉 Source Snowflake : Update JDBC driver for Snowflake to 3.13.22 #16766 is to override the setJsonField method in SnowflakeSourceOperations, and handle both types with the putTimestamp method, which works for timestamp:
      case TIMESTAMP, TIMESTAMP_WITH_TIMEZONE -> putTimestamp(json, columnName, resultSet, colIndex);
    • The fix is limited to Snowflake because we don't want to increase the scope of that PR.
  • The same issue could happen to time with timestamp as well, because that is also a standard JDBC type, and currently it falls under the default branch.

Expected Behavior

  • timestamp with timezone type should be handled correctly for all JDBC sources.
  • We need to investigate whether the solution for Snowflake works for other JDBC sources.
@bleonard
Copy link
Contributor

@akashkulk can you look into this as part of the data types work?

@akashkulk
Copy link
Contributor

The context here is that this is a refactoring issue. Need to move this into the AbstractClass + make sure all tests are the same

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

Successfully merging a pull request may close this issue.

5 participants