[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO

["Abacn (via GitHub)" <gi...@apache.org>]

Abacn commented on code in PR #28382:
URL: https://github.com/apache/beam/pull/28382#discussion_r1323313695


##########
sdks/python/apache_beam/io/jdbc.py:
##########
@@ -355,3 +359,89 @@ def __init__(
         ),
         expansion_service or default_io_expansion_service(classpath),
     )
+
+
+@LogicalType.register_logical_type
+class JdbcDateType(LogicalType[datetime.date, MillisInstant, str]):
+  """Support of Legacy JdbcIO DATE logical type."""
+  def __init__(self, argument=""):
+    pass
+
+  @classmethod
+  def representation_type(cls):
+    # type: () -> type
+    return Timestamp
+
+  @classmethod
+  def urn(cls):
+    return "beam:logical_type:javasdk_date:v1"
+
+  @classmethod
+  def language_type(cls):
+    return datetime.date
+
+  def to_representation_type(self, value):
+    # type: (datetime.date) -> Timestamp
+    return Timestamp.from_utc_datetime(

Review Comment:
   Here datetime.date and datetime.time object does not have timezone. This line is doing simple algebra given all timestamps are in UTC so it should not add additional issue. But because it is a wrapper for Java JdbcIO, same issue from Java is expected, specifically here
   
   https://github.com/apache/beam/blob/1f9febeff05bf1e151136bb49a53f74538d2a3a9/sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/SchemaUtil.java#L332
   
   Here (`Time time = rs.getTime`) using a legacy time converter that is implementation dependent (and macOS has issue). #28382 remains open (Date resolved in #22738 but Time issue remains) because we use Apache Derby in unit and integration tests, however, the latest version supporting Java 8 does not support java.time Time converter (it supported java.time Date converter though) so a change similar to #22738 for time broke Time (and persumably would also break some users on older database)
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org