[BEAM-11719 ] Use deterministic coders for grouping keys. #13839

robertwb · 2021-01-29T19:02:03Z

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).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
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.

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---		---	---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

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

codecov · 2021-02-01T21:30:38Z

Codecov Report

Merging #13839 (1318fbd) into master (c137e88) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13839      +/-   ##
==========================================
+ Coverage   82.97%   82.99%   +0.01%     
==========================================
  Files         469      469              
  Lines       58300    58325      +25     
==========================================
+ Hits        48377    48407      +30     
+ Misses       9923     9918       -5

Impacted Files	Coverage Δ
.../py38/build/srcs/sdks/python/apache_beam/pvalue.py
...ks/python/apache_beam/internal/metrics/__init__.py
...eam/runners/portability/fn_api_runner/execution.py
...hon/apache_beam/examples/cookbook/mergecontacts.py
...s/python/apache_beam/io/gcp/bigquery_file_loads.py
...testing/benchmarks/nexmark/queries/winning_bids.py
...dks/python/apache_beam/metrics/monitoring_infos.py
...ild/srcs/sdks/python/apache_beam/io/gcp/dicomio.py
.../apache_beam/runners/dataflow/internal/__init__.py
...python/apache_beam/runners/portability/__init__.py
... and 928 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f647b05...1318fbd. Read the comment docs.

…ion. Falling back to pickling for key grouping was never safe, and should not have been recommended.

robertwb · 2021-02-19T20:51:31Z

R: @udim
CC: @katsiapis

sdks/python/apache_beam/transforms/ptransform_test.py

sdks/python/apache_beam/runners/dataflow/dataflow_runner.py

sdks/python/apache_beam/examples/cookbook/group_with_coder_test.py

udim · 2021-02-24T02:48:50Z

sdks/python/apache_beam/io/gcp/bigquery_file_loads.py

            deleting_tables: deleting_tables,
            pvalue.AsIter(temp_tables_pc))
-        | "RemoveTempTables/AddUselessValue" >> beam.Map(lambda x: (x, None))
+        # TableReference has no deterministic coder, but as this de-duplication


From what I can tell, parse_table_reference is used to normalize and populate TableReference instances. They are essentially a "(project, dataset, table) tuple."
It should be possible to beam.Map(lambda x: (x.tableId, x.datasetId, x.projectId)) | beam.Distinct() | beam.Map(lambda x: parse_table_reference(*x))

cc: @pabloem

Assuming exactly these three fields would not be as future proof.

Ack. The official BigQuery client (which this code doesn't use) has a hashable wrapper for this.
https://googleapis.dev/python/bigquery/latest/generated/google.cloud.bigquery.table.TableReference.html

sdks/python/apache_beam/runners/pipeline_context.py

robertwb

Thanks, PTAL.

sdks/python/apache_beam/examples/cookbook/group_with_coder_test.py

robertwb · 2021-02-24T17:44:09Z

sdks/python/apache_beam/io/gcp/bigquery_file_loads.py

            deleting_tables: deleting_tables,
            pvalue.AsIter(temp_tables_pc))
-        | "RemoveTempTables/AddUselessValue" >> beam.Map(lambda x: (x, None))
+        # TableReference has no deterministic coder, but as this de-duplication


Assuming exactly these three fields would not be as future proof.

sdks/python/apache_beam/runners/pipeline_context.py

sdks/python/apache_beam/transforms/ptransform_test.py

robertwb · 2021-02-24T23:32:46Z

Run Python_PVR_Flink PreCommit

chunyang · 2021-02-27T08:25:32Z

I seem to be bumping into an error caused by deterministic coder enforcement. Do you mind taking a look at ##14112?

robertwb changed the title ~~Use deterministic coders for grouping keys.~~ [BEAM-11719 ] Use deterministic coders for grouping keys. Feb 1, 2021

robertwb force-pushed the structural-keys branch from ee5ef89 to 79301c4 Compare February 1, 2021 21:14

robertwb force-pushed the structural-keys branch from 79301c4 to c93696a Compare February 1, 2021 21:40

robertwb added 3 commits February 18, 2021 20:42

[BEAM-11719] Use deterministic coders for grouping keys.

fd61359

Fix bigquery file load.

8141e45

Fix GroupWithCoderTest.test_basics_without_type_check to check except…

98cc71f

…ion. Falling back to pickling for key grouping was never safe, and should not have been recommended.

robertwb force-pushed the structural-keys branch from c93696a to 98cc71f Compare February 19, 2021 04:42

robertwb requested a review from udim February 19, 2021 20:50

Add the ability to register additional fallback coders.

64adb6a

robertwb force-pushed the structural-keys branch from 4b9fb15 to 64adb6a Compare February 19, 2021 23:15

[BEAM-11719] Pipeline option for non-deterministic key coders.

fd87993

robertwb force-pushed the structural-keys branch from 42458d8 to fd87993 Compare February 23, 2021 01:22

Lint, import fix.

10abd33

udim reviewed Feb 24, 2021

View reviewed changes

robertwb added 2 commits February 24, 2021 09:48

Fixup

8f0b056

Merge remote-tracking branch 'github/master' into structural-keys

1318fbd

robertwb commented Feb 24, 2021

View reviewed changes

udim approved these changes Feb 25, 2021

View reviewed changes

robertwb merged commit d3f716d into apache:master Feb 25, 2021

chunyang mentioned this pull request Feb 27, 2021

[BEAM-11884] Pass destination as str rather than TableReference within BigQueryBatchFileLoads #14112

Merged

4 tasks

[BEAM-11719 ] Use deterministic coders for grouping keys. #13839

[BEAM-11719 ] Use deterministic coders for grouping keys. #13839

Uh oh!

Conversation

robertwb commented Jan 29, 2021

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

codecov bot commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

robertwb commented Feb 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

udim Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

robertwb Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

udim Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertwb Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertwb commented Feb 24, 2021

Uh oh!

chunyang commented Feb 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 1, 2021 •

edited

Loading