You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "Abacn (via GitHub)" <gi...@apache.org> on 2023/09/09 00:20:57 UTC
[GitHub] [beam] Abacn opened a new pull request, #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Abacn opened a new pull request, #28382:
URL: https://github.com/apache/beam/pull/28382
Part of #28359 (date and time support without breaking change); fixes #19715
* Add identifier to URN for legacy Java logical types
* Implement JdbcIO logical type javasdk_date and javasdk_time in Python
* Add tests
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "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:
Yes the issue was #21115.
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). #21115 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 would break Time conversion (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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "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 would break Time conversion (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
[GitHub] [beam] johnjcasey commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "johnjcasey (via GitHub)" <gi...@apache.org>.
johnjcasey commented on code in PR #28382:
URL: https://github.com/apache/beam/pull/28382#discussion_r1327677704
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaTranslation.java:
##########
@@ -429,15 +442,19 @@ private static FieldType fieldTypeFromProtoWithoutNullable(SchemaApi.FieldType p
} else if (urn.equals(URN_BEAM_LOGICAL_DECIMAL)) {
return FieldType.DECIMAL;
} else if (urn.startsWith("beam:logical_type:")) {
- try {
- return FieldType.logicalType(
- (LogicalType)
- SerializableUtils.deserializeFromByteArray(
- logicalType.getPayload().toByteArray(), "logicalType"));
- } catch (IllegalArgumentException e) {
- LOG.warn(
- "Unable to deserialize the logical type {} from proto. Mark as UnknownLogicalType.",
- urn);
+ if (!logicalType.getPayload().isEmpty()) {
Review Comment:
Can we raise an issue and add a TODO here to make sure it isn't lost?
##########
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]):
Review Comment:
We should add that in a comment in the code
--
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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1723620173
tests passed either on Jenkins or GitHub Action, flaky tests unrelated
--
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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1712387620
Run Python 3.11 PostCommit
--
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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1716318655
Run Python 3.11 PostCommit
--
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
[GitHub] [beam] github-actions[bot] commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1712385864
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @liferoad for label python.
R: @bvolpato for label java.
R: @ahmedabu98 for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28382:
URL: https://github.com/apache/beam/pull/28382#discussion_r1326329453
##########
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]):
Review Comment:
these logical types handles legacy DATE and TIME type from Java SDK, it was not defined in the original portable schema doc i.e., not standard logical type. So put them under jdbc.py instead of typehint/schemas.py (place for standard logical type). Java side these types are also in JdbcIO module (not in java core)
--
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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1719902638
R: @chamikaramj @johnjcasey for Java side
--
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
[GitHub] [beam] liferoad commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #28382:
URL: https://github.com/apache/beam/pull/28382#discussion_r1320612105
##########
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:
I remember we had one issue before related to the timezone. do we always assume value is in UTC?
--
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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "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:
Yes the issue was #28382.
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 would break Time conversion (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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1716093211
Run Python 3.11 PostCommit
--
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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1716212288
Run Python 3.11 PostCommit
--
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
[GitHub] [beam] codecov[bot] commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1712364137
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/28382?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#28382](https://app.codecov.io/gh/apache/beam/pull/28382?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2cff623) into [master](https://app.codecov.io/gh/apache/beam/commit/03b5588d4e61c3092cdad83dff66703be0527554?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (03b5588) will **decrease** coverage by `0.01%`.
> The diff coverage is `73.58%`.
```diff
@@ Coverage Diff @@
## master #28382 +/- ##
==========================================
- Coverage 72.33% 72.33% -0.01%
==========================================
Files 682 682
Lines 100536 100588 +52
==========================================
+ Hits 72727 72759 +32
- Misses 26233 26253 +20
Partials 1576 1576
```
| [Flag](https://app.codecov.io/gh/apache/beam/pull/28382/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [python](https://app.codecov.io/gh/apache/beam/pull/28382/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.85% <73.58%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/beam/pull/28382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [sdks/python/apache\_beam/io/jdbc.py](https://app.codecov.io/gh/apache/beam/pull/28382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vamRiYy5weQ==) | `75.00% <73.58%> (-4.17%)` | :arrow_down: |
... and [8 files with indirect coverage changes](https://app.codecov.io/gh/apache/beam/pull/28382/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1712529324
Run Python 3.11 PostCommit
--
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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1721847530
Thanks, added comments to both paths
--
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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28382:
URL: https://github.com/apache/beam/pull/28382#discussion_r1326337136
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaTranslation.java:
##########
@@ -429,15 +442,19 @@ private static FieldType fieldTypeFromProtoWithoutNullable(SchemaApi.FieldType p
} else if (urn.equals(URN_BEAM_LOGICAL_DECIMAL)) {
return FieldType.DECIMAL;
} else if (urn.startsWith("beam:logical_type:")) {
- try {
- return FieldType.logicalType(
- (LogicalType)
- SerializableUtils.deserializeFromByteArray(
- logicalType.getPayload().toByteArray(), "logicalType"));
- } catch (IllegalArgumentException e) {
- LOG.warn(
- "Unable to deserialize the logical type {} from proto. Mark as UnknownLogicalType.",
- urn);
+ if (!logicalType.getPayload().isEmpty()) {
Review Comment:
leave as future improvement - currently change to DEBUG log to reduce noise
--
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
[GitHub] [beam] github-actions[bot] commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1719914078
## Test Results
3 files - 143 3 suites - 143 7s [:stopwatch:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.9.0/README.md#the-symbols "duration of all tests") - 39m 5s
35 tests - 1 299 33 [:heavy_check_mark:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.9.0/README.md#the-symbols "passed tests") - 1 268 2 [:zzz:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.9.0/README.md#the-symbols "skipped / disabled tests") - 31 0 [:x:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.9.0/README.md#the-symbols "failed tests") ±0
35 runs - 1 301 33 [:heavy_check_mark:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.9.0/README.md#the-symbols "passed tests") - 1 270 2 [:zzz:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.9.0/README.md#the-symbols "skipped / disabled tests") - 31 0 [:x:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.9.0/README.md#the-symbols "failed tests") ±0
Results for commit 1d965649. ± Comparison against base commit 03b5588d.
<details>
<summary>This pull request <b>removes</b> 1336 and <b>adds</b> 35 tests. <i>Note that renamed tests count towards both.</i></summary>
```
id]
id`]
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testDenseRankFunction
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testFirstValueFunction
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testLastValueFunction
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testOverCumulativeSum
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testOverCumulativeSumOrderByDesc
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testOverRangeBoundedSum
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testOverRowsBoundedSum
org.apache.beam.sdk.extensions.sql.BeamAnalyticFunctionsTest ‑ testPercentRankFunction
…
```
```
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testAzfsResourceIdToString
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testContainerParsing
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testEquals
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testFromComponents
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testFromUri
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testGetCurrentDirectory
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testGetFilename
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testGetScheme
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testInvalidAzfsResourceId
org.apache.beam.sdk.io.azure.blobstore.AzfsResourceIdTest$NonParameterizedTests ‑ testInvalidContainer
…
```
</details>
<details>
<summary>This pull request <b>removes</b> 33 skipped tests and <b>adds</b> 2 skipped tests. <i>Note that renamed tests count towards both.</i></summary>
```
org.apache.beam.sdk.extensions.sql.BeamComplexTypeTest ‑ testNestedArrayOfBytes
org.apache.beam.sdk.extensions.sql.BeamComplexTypeTest ‑ testNestedBytes
org.apache.beam.sdk.extensions.sql.BeamComplexTypeTest ‑ testSelectInnerRowOfNestedRow
org.apache.beam.sdk.extensions.sql.BeamSqlDslAggregationTest ‑ testBitAndFunctionWithNullInputAndValueInput
org.apache.beam.sdk.extensions.sql.BeamSqlDslSqlStdOperatorsTest ‑ testThatAllOperatorsAreTested
org.apache.beam.sdk.extensions.sql.impl.JdbcDriverTest ‑ testTimestampWithNonzeroTimezone
org.apache.beam.sdk.extensions.sql.impl.rel.BeamMatchRelTest ‑ matchNFATest
org.apache.beam.sdk.extensions.sql.integrationtest.BeamSqlDateFunctionsIntegrationTest ‑ testDateTimeFunctions_currentTime
org.apache.beam.sdk.extensions.sql.zetasql.StreamingSqlTest ‑ testZetaSQLStructFieldAccessInTumble
org.apache.beam.sdk.extensions.sql.zetasql.ZetaSqlDialectSpecTest ‑ testCastBetweenTimeAndString
…
```
```
org.apache.beam.sdk.io.azure.blobstore.AzureBlobStoreFileSystemTest ‑ testGlobExpansion
org.apache.beam.sdk.io.azure.blobstore.AzureBlobStoreFileSystemTest ‑ testMatch
```
</details>
[test-results]:data:application/gzip;base64,H4sIAPRLA2UC/1WMyw7CIBBFf6Vh7UKkDI4/Y5BHQmyL4bEy/rtTVFp395ybnCfzYXKZXQZxGFiuoXSwNekS4kKoCOko7ZI/uOZqzGrEZu7hQebUhddhInHswqUU09ekuvTiuv+CH7H1Gu9yjfc1E+c5FALGLYKEEeHMARCsuo0SPXqtAJTQnFuprXPIXm8kVFEY/wAAAA==
--
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
[GitHub] [beam] github-actions[bot] commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1719904120
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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
[GitHub] [beam] Abacn merged pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #28382:
URL: https://github.com/apache/beam/pull/28382
--
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
[GitHub] [beam] liferoad commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #28382:
URL: https://github.com/apache/beam/pull/28382#discussion_r1323400224
##########
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:
can we add comments here to have these details?
--
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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28382:
URL: https://github.com/apache/beam/pull/28382#discussion_r1323487800
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaTranslation.java:
##########
@@ -429,15 +442,19 @@ private static FieldType fieldTypeFromProtoWithoutNullable(SchemaApi.FieldType p
} else if (urn.equals(URN_BEAM_LOGICAL_DECIMAL)) {
return FieldType.DECIMAL;
} else if (urn.startsWith("beam:logical_type:")) {
- try {
- return FieldType.logicalType(
- (LogicalType)
- SerializableUtils.deserializeFromByteArray(
- logicalType.getPayload().toByteArray(), "logicalType"));
- } catch (IllegalArgumentException e) {
- LOG.warn(
- "Unable to deserialize the logical type {} from proto. Mark as UnknownLogicalType.",
- urn);
+ if (!logicalType.getPayload().isEmpty()) {
Review Comment:
Note for myself - I see verbose log here. We should cache the UnknownLogicalType if it has a urn.
--
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
[GitHub] [beam] Abacn commented on a diff in pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "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:
Yes the issue was #21115.
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). #21115 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 converter and there are test breaks if similar approach applied
--
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
[GitHub] [beam] Abacn commented on pull request #28382: Support legacy DATE and TIME logical types in xlang JdbcIO
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28382:
URL: https://github.com/apache/beam/pull/28382#issuecomment-1716157498
Run Python 3.11 PostCommit
--
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