You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "prodriguezdefino (via GitHub)" <gi...@apache.org> on 2024/10/18 23:20:34 UTC
[PR] [WIP] Enable configuration of mutation info callable for CDC Writes into BigQuery [beam]
prodriguezdefino opened a new pull request, #32878:
URL: https://github.com/apache/beam/pull/32878
**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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32878:
URL: https://github.com/apache/beam/pull/32878#issuecomment-2497854426
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @jrmccluskey for label python.
R: @damccorm for label build.
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)
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #32878: Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery
URL: https://github.com/apache/beam/pull/32878
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #32878:
URL: https://github.com/apache/beam/pull/32878#discussion_r1866186039
##########
sdks/python/apache_beam/io/gcp/bigquery.py:
##########
@@ -2550,7 +2577,7 @@ def __init__(
use_at_least_once=False,
with_auto_sharding=False,
num_storage_api_streams=0,
- use_cdc_writes: bool = False,
+ use_cdc_writes: UseCdcWrites = False,
Review Comment:
But lets start with the first question, because I'm not sure about the change as a whole yet
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "prodriguezdefino (via GitHub)" <gi...@apache.org>.
prodriguezdefino commented on code in PR #32878:
URL: https://github.com/apache/beam/pull/32878#discussion_r1866356320
##########
sdks/python/apache_beam/io/gcp/bigquery.py:
##########
@@ -2550,7 +2577,7 @@ def __init__(
use_at_least_once=False,
with_auto_sharding=False,
num_storage_api_streams=0,
- use_cdc_writes: bool = False,
+ use_cdc_writes: UseCdcWrites = False,
Review Comment:
Originally this change was part of #32529 and given the push to get it included for the v2.60.0 release @ahmedabu98 recommended partition it in two different PRs.
Before #32529 Python SDK was not capable of using CDC writes into BQ and this change brings parity with BigQueryIO by exposing a lambda to include Row Mutation information as part of the row to be ingested. Currently, a SDK user would need to know how to structure their Rows to be ingested using CDC or, if they use Dicts as their data format, their provided schema should include the row mutation information making it not matching with the actual BigQuery table schema they want to write to (otherwise the xlang protocol wouldn't work).
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #32878:
URL: https://github.com/apache/beam/pull/32878#discussion_r1866473483
##########
sdks/python/apache_beam/io/gcp/bigquery.py:
##########
@@ -2550,7 +2577,7 @@ def __init__(
use_at_least_once=False,
with_auto_sharding=False,
num_storage_api_streams=0,
- use_cdc_writes: bool = False,
+ use_cdc_writes: UseCdcWrites = False,
Review Comment:
> Currently, a SDK user would need to know how to structure their Rows to be ingested using CDC or, if they use Dicts as their data format, their provided schema should include the row mutation information making it not matching with the actual BigQuery table schema they want to write to (otherwise the xlang protocol wouldn't work).
Isn't this still true with your change? The only difference is that they're now encapsulating the conversion logic in their Write transform instead of having a step to do it beforehand. Basically, as I understand it, the following 2 blocks are equivalent (and not particularly different to write):
```
pcoll
| WriteToBigQuery(..., use_cdc_writes=my_fn, ...)
```
and:
```
pcoll
| beam.Map(my_fn)
| WriteToBigQuery(..., use_cdc_writes=True, ...)
```
the main difference is that the 2nd is easier to debug IMO. Am I right that those are equivalent in all cases or am I missing something?
> this change brings parity with BigQueryIO
This is a good reason to consider the change, but not enough to add it IMO
> We have similar overloads in this same file, see [here](https://github.com/apache/beam/blob/master/sdks/python/apache_beam/io/gcp/bigquery.py#L972) and also we have other instances on where the overload is silently added on a [typeless argument](https://github.com/apache/beam/blob/master/sdks/python/apache_beam/io/gcp/bigquery.py#L2571).
To be honest, I don't love that usage either. However, there are a few differences there:
1) It is not a user facing API
2) `table` is a more general arg than `use_cdc_writes` (which is no longer descriptive with this change)
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #32878:
URL: https://github.com/apache/beam/pull/32878#discussion_r1866185236
##########
sdks/python/apache_beam/io/gcp/bigquery.py:
##########
@@ -2550,7 +2577,7 @@ def __init__(
use_at_least_once=False,
with_auto_sharding=False,
num_storage_api_streams=0,
- use_cdc_writes: bool = False,
+ use_cdc_writes: UseCdcWrites = False,
Review Comment:
Could you add some more context on why we need this PR? What use case is it enabling which previously was impossible/is there a reason having this as a parameter is better than just recommending users do preprocessing before the write step? As much as we can, I'd prefer to keep custom user code out of the IO for debugability.
I'll also note that this parameter is now very overloaded. I'd probably prefer an extra param here over repurposing this one.
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32878:
URL: https://github.com/apache/beam/pull/32878#issuecomment-2490977640
Reminder, please take a look at this pr: @shunping @Abacn @johnjcasey
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32878:
URL: https://github.com/apache/beam/pull/32878#issuecomment-2471852819
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @shunping for label python.
R: @Abacn for label build.
R: @johnjcasey 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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32878:
URL: https://github.com/apache/beam/pull/32878#issuecomment-2633803548
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32878:
URL: https://github.com/apache/beam/pull/32878#issuecomment-2520155203
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @liferoad for label python.
R: @Abacn for label build.
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)
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32878:
URL: https://github.com/apache/beam/pull/32878#issuecomment-2511378307
Reminder, please take a look at this pr: @jrmccluskey @damccorm @ahmedabu98
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "prodriguezdefino (via GitHub)" <gi...@apache.org>.
prodriguezdefino commented on code in PR #32878:
URL: https://github.com/apache/beam/pull/32878#discussion_r1866368002
##########
sdks/python/apache_beam/io/gcp/bigquery.py:
##########
@@ -2550,7 +2577,7 @@ def __init__(
use_at_least_once=False,
with_auto_sharding=False,
num_storage_api_streams=0,
- use_cdc_writes: bool = False,
+ use_cdc_writes: UseCdcWrites = False,
Review Comment:
> I'll also note that this parameter is now very overloaded. I'd probably prefer an extra param here over repurposing this one.
We have similar overloads in this same file, see [here](https://github.com/apache/beam/blob/master/sdks/python/apache_beam/io/gcp/bigquery.py#L972) and also we have other instances on where the overload is silently added on a [typeless argument](https://github.com/apache/beam/blob/master/sdks/python/apache_beam/io/gcp/bigquery.py#L2571).
The strict type definition simplifies the understanding of the shape for the argument, and to some degree the validations made later (would have been even simpler and more strict with Python > 3.10).
--
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
Re: [PR] Enable configuration of a CDC mutation info Callable for CDC Writes into BigQuery [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32878:
URL: https://github.com/apache/beam/pull/32878#issuecomment-2650719824
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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