Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable #23548

Abacn · 2022-10-07T22:54:07Z

Fixes #23526

Please add a meaningful description for your change here

This enables cross-language jdbc ptransform write and read fixed/variable-length strings and bytes

Move to portable logical types in Java
Create portable logical types in Python

Python cross-language JdbcIO can read/write rows with CHAR, VARCHAR, BINARY, VARBINARY types now.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
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.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

Abacn · 2022-10-08T02:45:29Z

Run Python 3.8 PostCommit

Abacn · 2022-10-08T03:08:46Z

jdbc xlang test succeeded: https://ci-beam.apache.org/job/beam_PostCommit_Python38_PR/632/testReport/

Will clean up the changes and mark as open

* Move jdbc logical type to portable logical types in Java * Create portable logical types in Python * Support value_from_runner_api and value_to_runner_api in Python SchemaTransform (currently only support atomic type values) Fix nullable/test/leftovers

codecov · 2022-10-08T13:32:26Z

Codecov Report

Merging #23548 (e0a14e4) into master (8216cba) will decrease coverage by 0.39%.
The diff coverage is 85.08%.

@@            Coverage Diff             @@
##           master   #23548      +/-   ##
==========================================
- Coverage   73.45%   73.06%   -0.40%     
==========================================
  Files         718      728      +10     
  Lines       95884    96413     +529     
==========================================
+ Hits        70435    70443       +8     
- Misses      24138    24659     +521     
  Partials     1311     1311

Flag	Coverage Δ
python	`82.57% <85.08%> (-0.65%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdks/python/apache_beam/coders/coder_impl.py	`93.77% <33.33%> (-0.17%)`	⬇️
sdks/python/apache_beam/typehints/schemas.py	`88.95% <85.63%> (-2.94%)`	⬇️
sdks/python/apache_beam/portability/common_urns.py	`100.00% <100.00%> (ø)`
sdks/python/apache_beam/io/gcp/bigquery_tools.py	`73.34% <0.00%> (-12.39%)`	⬇️
...python/apache_beam/io/gcp/bigquery_schema_tools.py	`79.24% <0.00%> (-4.85%)`	⬇️
.../python/apache_beam/testing/test_stream_service.py	`88.09% <0.00%> (-4.77%)`	⬇️
sdks/python/apache_beam/io/gcp/bigquery.py	`71.11% <0.00%> (-3.13%)`	⬇️
sdks/python/apache_beam/internal/gcp/json_value.py	`85.50% <0.00%> (-2.90%)`	⬇️
sdks/python/apache_beam/runners/runner.py	`72.39% <0.00%> (-1.85%)`	⬇️
.../python/apache_beam/transforms/periodicsequence.py	`98.38% <0.00%> (-1.62%)`	⬇️
... and 46 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

* Fix RowCoderImpl cannot encode bytes column in cython compiled * Set coder_impl.is_compiled=True when running on compiled stream module

github-actions · 2022-10-10T12:47:54Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @AnandInguva for label python.
R: @apilloud for label java.
R: @jrmccluskey for label go.
R: @chamikaramj 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).

Abacn · 2022-10-10T13:26:07Z

R: @TheNeuralBit

github-actions · 2022-10-10T13:38:59Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

Abacn · 2022-10-10T13:41:51Z

Apologize for a large PR. Most changes are necessary refactors had to do, hopefully they looks trivial and not hard to review

In Java, changed some switch clause to if-else clause that involves Logicaltype.IDENTIFIER defined in proto as they are no longer contant at compile time and cannot use switch.
In Python, refactored option_from/to_runner_api, using their logic to create value_from/to_runner_api as logical type argument values also use them.

Also included some fix found that not catched before

AvroUtil logical type translation did not consider VARBINARY (now VariableBytes)
cythonized RowColumnEncoder cannot handle bytes column becuase nparray([bytes]) element types are np.bytes_ and cython does not recognize it as as bytes
coder_impl.is_compiled flag is always false when cythonized stream module exist

jrmccluskey · 2022-10-10T14:30:17Z

Run XVR_Flink PostCommit

jrmccluskey · 2022-10-10T14:34:09Z

Run XVR_Direct PostCommit

Abacn · 2022-10-10T14:51:39Z

"Gradle build daemon disappeared unexpectedly" for Direct CrossLanguageValidatesRunner Tests

Update: second time failure is

11:29:40 FAILED apache_beam/transforms/sql_test.py::SqlTransformTest::test_filter - gr...
11:29:40 = 1 failed, 8 passed, 3 skipped, 6670 deselected, 1 warning in 381.99s (0:06:21) =
11:29:40 Exception in thread wait_until_finish_read:
11:29:40 Traceback (most recent call last):
...
11:29:40 grpc._channel._MultiThreadedRendezvous: <_MultiThreadedRendezvous of RPC that terminated with:
11:29:40 	status = StatusCode.DEADLINE_EXCEEDED
11:29:40 	details = "Deadline Exceeded"
11:29:40 	debug_error_string = "UNKNOWN:Error received from peer ipv4:127.0.0.1:33525 {created_time:"2022-10-10T15:25:37.535284357+00:00", grpc_status:4, grpc_message:"Deadline Exceeded"}"

seems like another unrelated grpc flake

Abacn · 2022-10-10T14:51:47Z

Run XVR_Direct PostCommit

TheNeuralBit

Thanks @Abacn this is very thorough and LGTM overall. I just have a few minor suggestions on the Python side.

TheNeuralBit · 2022-10-26T18:16:44Z

sdks/python/apache_beam/typehints/schemas.py

+        # TODO(https://github.com/apache/beam/issues/23373): Complete support
+        # for logical types that require arguments beyond atomic type.
+        # For now, skip arguments.
+        return logical_type()


I think it would be better to fail loudly here. It's dangerous to move forward with a type we don't understand.

Are there cases where we need to let this through?

Yeah, this was the behavior before (arguments omitted) and the fixed_decimal logical type has a Row type argument. It is special because the scale argument is encoded both in argument and in its representation, so that I do not need the argument to recover the data from bytes. The argument's usage is to enable adding a guard for overflow check (like for fixed_char and fixed_bytes) here. Since the same check is done in Java side so it is safe so far.

Added a warning here.

TheNeuralBit · 2022-10-26T18:18:07Z

sdks/python/apache_beam/typehints/schemas.py

+  @classmethod
+  def _from_typing(cls, typ):
+    # type: (type) -> LogicalType
+    # TODO: enable argument


can we link an issue here (it could be the same one for following up on non-atomic types)

Thanks, linked to #23373

TheNeuralBit · 2022-10-26T18:19:50Z

sdks/python/apache_beam/typehints/schemas.py

+    return np.int32
+
+  def argument(self):
+    return self.max_length


Could you document the type mappings for these in the docstring for this module?

Added in schemas module docstring

Abacn · 2022-10-27T17:17:30Z

Flaky python precommit tests related to #22115
Flaky Java precommit tests #21714

Abacn · 2022-10-27T17:53:42Z

Run Python 3.9 PostCommit

Abacn · 2022-10-27T18:51:48Z

Run Java PreCommit

Abacn · 2022-10-27T21:43:46Z

xlang integration test passed: https://ci-beam.apache.org/job/beam_PostCommit_Python39_PR/94/testReport/apache_beam.io.external.xlang_jdbcio_it_test/CrossLanguageJdbcIOTest/

…le (apache#23548) * Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable * Move jdbc logical type to portable logical types in Java * Create portable logical types in Python * Support value_from_runner_api and value_to_runner_api in Python SchemaTransform (currently only support atomic type values) Fix nullable/test/leftovers * Fix typos * Add standard coder test * Fix RowCoderImpl cannot encode bytes column in cython compiled * Set coder_impl.is_compiled=True when running on compiled stream module * Add docstring, add todo and warnings for unsupported

Abacn · 2022-10-28T14:21:52Z

Thanks @TheNeuralBit !

…le (apache#23548) * Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable * Move jdbc logical type to portable logical types in Java * Create portable logical types in Python * Support value_from_runner_api and value_to_runner_api in Python SchemaTransform (currently only support atomic type values) Fix nullable/test/leftovers * Fix typos * Add standard coder test * Fix RowCoderImpl cannot encode bytes column in cython compiled * Set coder_impl.is_compiled=True when running on compiled stream module * Add docstring, add todo and warnings for unsupported

github-actions bot added build io java jdbc model python parquet labels Oct 7, 2022

Abacn force-pushed the portablevarchar branch from 17488de to 140ba6d Compare October 8, 2022 01:20

github-actions bot removed the parquet label Oct 8, 2022

Abacn force-pushed the portablevarchar branch 2 times, most recently from f2f86f2 to 91307b1 Compare October 8, 2022 02:45

Abacn force-pushed the portablevarchar branch from 91307b1 to 35b253e Compare October 8, 2022 03:15

github-actions bot removed the build label Oct 8, 2022

Fix typos

d22c635

github-actions bot added the go label Oct 9, 2022

Add standard coder test

b3d9d72

* Fix RowCoderImpl cannot encode bytes column in cython compiled * Set coder_impl.is_compiled=True when running on compiled stream module

Abacn force-pushed the portablevarchar branch from 7f43d6e to b3d9d72 Compare October 9, 2022 11:33

Abacn marked this pull request as ready for review October 10, 2022 12:35

github-actions bot added the Next Action: Reviewers label Oct 10, 2022

TheNeuralBit approved these changes Oct 26, 2022

View reviewed changes

Add docstring, add todo and warnings for unsupported

e0a14e4

TheNeuralBit merged commit 29ff4be into apache:master Oct 27, 2022

Abacn deleted the portablevarchar branch October 28, 2022 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable #23548

Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable #23548

Abacn commented Oct 7, 2022 •

edited

Loading

Abacn commented Oct 8, 2022

Abacn commented Oct 8, 2022 •

edited

Loading

codecov bot commented Oct 8, 2022 •

edited

Loading

github-actions bot commented Oct 10, 2022

Abacn commented Oct 10, 2022

github-actions bot commented Oct 10, 2022

Abacn commented Oct 10, 2022 •

edited

Loading

jrmccluskey commented Oct 10, 2022

jrmccluskey commented Oct 10, 2022

Abacn commented Oct 10, 2022 •

edited

Loading

Abacn commented Oct 10, 2022

TheNeuralBit left a comment

TheNeuralBit Oct 26, 2022

Abacn Oct 27, 2022 •

edited

Loading

TheNeuralBit Oct 26, 2022

Abacn Oct 27, 2022

TheNeuralBit Oct 26, 2022

Abacn Oct 27, 2022

Abacn commented Oct 27, 2022 •

edited

Loading

Abacn commented Oct 27, 2022

Abacn commented Oct 27, 2022

Abacn commented Oct 27, 2022

Abacn commented Oct 28, 2022

Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable #23548

Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable #23548

Conversation

Abacn commented Oct 7, 2022 • edited Loading

GitHub Actions Tests Status (on master branch)

Abacn commented Oct 8, 2022

Abacn commented Oct 8, 2022 • edited Loading

codecov bot commented Oct 8, 2022 • edited Loading

Codecov Report

github-actions bot commented Oct 10, 2022

Abacn commented Oct 10, 2022

github-actions bot commented Oct 10, 2022

Abacn commented Oct 10, 2022 • edited Loading

jrmccluskey commented Oct 10, 2022

jrmccluskey commented Oct 10, 2022

Abacn commented Oct 10, 2022 • edited Loading

Abacn commented Oct 10, 2022

TheNeuralBit left a comment

Choose a reason for hiding this comment

TheNeuralBit Oct 26, 2022

Choose a reason for hiding this comment

Abacn Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

TheNeuralBit Oct 26, 2022

Choose a reason for hiding this comment

Abacn Oct 27, 2022

Choose a reason for hiding this comment

TheNeuralBit Oct 26, 2022

Choose a reason for hiding this comment

Abacn Oct 27, 2022

Choose a reason for hiding this comment

Abacn commented Oct 27, 2022 • edited Loading

Abacn commented Oct 27, 2022

Abacn commented Oct 27, 2022

Abacn commented Oct 27, 2022

Abacn commented Oct 28, 2022

Abacn commented Oct 7, 2022 •

edited

Loading

Abacn commented Oct 8, 2022 •

edited

Loading

codecov bot commented Oct 8, 2022 •

edited

Loading

Abacn commented Oct 10, 2022 •

edited

Loading

Abacn commented Oct 10, 2022 •

edited

Loading

Abacn Oct 27, 2022 •

edited

Loading

Abacn commented Oct 27, 2022 •

edited

Loading