You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "claudevdm (via GitHub)" <gi...@apache.org> on 2025/03/23 17:42:00 UTC
[PR] Add ability to specify partition bounds via jdbc xlang. [beam]
claudevdm opened a new pull request, #34394:
URL: https://github.com/apache/beam/pull/34394
This allows bypassing the requirement to grant database access to machine where pipeline is built/submitted for partition bound inference.
------------------------
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2749120689
> > we can just change the JdbcIO.read transform (pointed above, JdbcIO.java#L1513) added by jdbcio.readfrompartition to explicitly set a schema (Long), this is a much smaller change than introducing parameters, and user does not need to set these parameters also
>
> sorry, I see, this is a JdbcIO.read instead of JdbcIO.readRows, so it does not need database connection at expansion time. Where is the database connection happens currently?
Hmm, I actually thought it tries to get the partition bounds during pipeline expand, but looking again I was mistaken :D. Thanks for pointing this out.
So this change isnt really necessary to avoid database access during expansion. But we might as well add it to give the ability to explicitly pass partition bounds to avoid extra database calls during execution.
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm closed pull request #34394: Add ability to specify partition bounds via jdbc xlang.
URL: https://github.com/apache/beam/pull/34394
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2748638867
also if the purpose is
> bypassing the requirement to grant database access to machine where pipeline is built/submitted for partition bound inference.
we should be able to change here
https://github.com/apache/beam/blob/3595a3303df6d2e4c8e77482ca8120994d0d5881/sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java#L1513
to use explicit schema, if partition type is set at pipeline expansion 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
Re: [PR] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2010471432
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -72,6 +72,8 @@ public Schema configurationSchema() {
// readQuery
.addNullableField("partitionColumn", FieldType.STRING)
.addNullableField("partitions", FieldType.INT16)
+ .addNullableField("longLowerBound", FieldType.INT64)
Review Comment:
This only works for long. Currently there is builtin support for Long and Datetime. I am thinking about if it would benefits from a more flexible parameter design, e.g. pass in string and partition type names as SchemaIO parameters
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2748665701
> also if the purpose is
>
> > bypassing the requirement to grant database access to machine where pipeline is built/submitted for partition bound inference.
>
> we should be able to change here
>
> https://github.com/apache/beam/blob/3595a3303df6d2e4c8e77482ca8120994d0d5881/sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java#L1513
>
> to use explicit schema, if partition type is set at pipeline expansion time (and not need to pass in lower and upper bound)
I don't follow. From my experience there are two cases where the database is accessed during expansion time:
1. Infer schema
2. Get partition bounds
For ReadWithPartitions, both partition bounds and schema need to be explicitly provided to avoid database queries during expansion time. There is already support for explicitly providing schema https://github.com/apache/beam/blob/3595a3303df6d2e4c8e77482ca8120994d0d5881/sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java#L1562
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2010736225
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -72,6 +72,8 @@ public Schema configurationSchema() {
// readQuery
.addNullableField("partitionColumn", FieldType.STRING)
.addNullableField("partitions", FieldType.INT16)
+ .addNullableField("longLowerBound", FieldType.INT64)
Review Comment:
Sounds good, I am fine with both. There is a chance introducing new configurations are not needed at all, please see comments https://github.com/apache/beam/pull/34394#issuecomment-2749017454
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2010484131
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -72,6 +72,8 @@ public Schema configurationSchema() {
// readQuery
.addNullableField("partitionColumn", FieldType.STRING)
.addNullableField("partitions", FieldType.INT16)
+ .addNullableField("longLowerBound", FieldType.INT64)
Review Comment:
I think it is simpler to have strict types in the JdbcSchemaIoProvider for partitions, and in python [1] we can change lower_bound to be Union[int, datetime]. Then we just set the relevant type in the crosslang config, the user doesnt need to do anything special.
But I decided not to do this right now because
1. Right now crosslang only supports long partitions. This is because it uses the default ReadWithPartitions method [2]
2. I am thinking majority of people are ok with using long partitions since xlang never even supported dates to begin with
Maybe we can add support for date partitions in a followup?
[1] https://github.com/apache/beam/blob/8b02feb678d4feef47212560b2d863ce6cc6bdb4/sdks/python/apache_beam/io/jdbc.py#L308
[2] https://github.com/apache/beam/blob/3595a3303df6d2e4c8e77482ca8120994d0d5881/sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java#L407
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2749017454
> there are two cases where the database is accessed during expansion time:
>
> 2. Get partition bounds
This is because Get partition bounds is done by a JdbcIO.read transform (pointed above, JdbcIO.java#L1513), as a result, it tries to infer a schema.
But this is not needed if we already know it would be a long. I mean we can set schema to FieldType.Long here, if we know it's going to be long,
then Get partition bounds and JdbcIO.readFromPartition won't require database connection at pipeline submission 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
Re: [PR] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2780690346
Reminder, please take a look at this pr: @liferoad @kennknowles @Abacn @shunping
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2816680706
Reminder, please take a look at this pr: @jrmccluskey @chamikaramj @damccorm @shunping
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2010728937
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -72,6 +72,8 @@ public Schema configurationSchema() {
// readQuery
.addNullableField("partitionColumn", FieldType.STRING)
.addNullableField("partitions", FieldType.INT16)
+ .addNullableField("longLowerBound", FieldType.INT64)
Review Comment:
I think keeping stricter types here simplifies things because read with partitions only supports dates and integers, if we think there is a chance we will add more partition types then a more flexible approach might be worth it? If you feel strongly about this I can change this to the flexible approach.
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2749098821
> we can just change the JdbcIO.read transform (pointed above, JdbcIO.java#L1513) added by jdbcio.readfrompartition to explicitly set a schema (Long), this is a much smaller change than introducing parameters, and user does not need to set these parameters also
sorry, I see, this is a JdbcIO.read instead of JdbcIO.readRows, so it does not need database connection at expansion time. Where is the database connection happens currently?
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2018765916
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -72,6 +72,8 @@ public Schema configurationSchema() {
// readQuery
.addNullableField("partitionColumn", FieldType.STRING)
.addNullableField("partitions", FieldType.INT16)
+ .addNullableField("longLowerBound", FieldType.INT64)
Review Comment:
Sorry for the delay, thought I'd chimed in but looks like I didn't finish the review.
Thoughts:
1) I think like Kenn mentioned below, omitting the `long` from these params seems reasonable. If we need to support more non-default types later, we could do so with postfixes, but it seems like making long the default is fine.
2) An alternative option for supporting this in x-lang is to make these a row/schema'd object which takes different types as fields (e.g. `lowerBound=Row('longLowerBound': 123, 'dateLowerBound': null)`). This might be easier/cleaner to evolve, though it is harder to map to yaml. I don't love this option.
3) Another alternative would be something like: `lowerBound='123', 'upperBound'='456', 'boundType'='long')` where we take the boundaries as strings and convert them to the right type. Since we only accept `long` as a bound type for now, we could omit this param. I kinda like this as a way to evolve this over time since it is pretty x-lang/yaml friendly and lets us evolve it over time. Plus we can still provide good typing in the python wrapper. `boundType` might not even be necessary in the long term if we can auto-infer the type (we could at least do this if we only evolve to long/date)
Overall, I think we should have a plan on how we plan to evolve this if we need to support date columns or abitrary other column types, I don't think it is safe to assume we will never need this. (3) is an option, but I'm open to other paths here if folks have ideas.
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2824089593
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: @kennknowles for label java.
R: @Abacn for label build.
R: @shunping 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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2749147248
> But we might as well add it to give the ability to explicitly pass partition bounds to avoid extra database calls during execution.
For now I am marginally fine with the change. For decisions of publicly faced API, good to involve additional reviewer to have an opinion?
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2789499886
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: @chamikaramj for label java.
R: @damccorm for label build.
R: @shunping 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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2037504411
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -72,6 +72,8 @@ public Schema configurationSchema() {
// readQuery
.addNullableField("partitionColumn", FieldType.STRING)
.addNullableField("partitions", FieldType.INT16)
+ .addNullableField("longLowerBound", FieldType.INT64)
Review Comment:
Any thoughts on these different approaches?
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34394:
URL: https://github.com/apache/beam/pull/34394#issuecomment-2748832987
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @liferoad for label python.
R: @kennknowles for label java.
R: @Abacn for label build.
R: @shunping 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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2012738099
##########
sdks/java/io/jdbc/src/test/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProviderTest.java:
##########
@@ -131,6 +131,31 @@ public void testPartitionedReadWithExplicitSchema() {
pipeline.run();
}
+ @Test
+ public void testPartitionedReadWithExplicitLongBounds() {
+ JdbcSchemaIOProvider provider = new JdbcSchemaIOProvider();
+
+ Row config =
+ Row.withSchema(provider.configurationSchema())
+ .withFieldValue("driverClassName", DATA_SOURCE_CONFIGURATION.getDriverClassName().get())
+ .withFieldValue("jdbcUrl", DATA_SOURCE_CONFIGURATION.getUrl().get())
+ .withFieldValue("username", "")
+ .withFieldValue("password", "")
+ .withFieldValue("partitionColumn", "id")
+ .withFieldValue("partitions", (short) 5)
+ .withFieldValue("longLowerBound", 0L)
Review Comment:
nit: leave the type out of the field name
These names are user-facing (unless we do work to hide them) in managed IO, YAML, etc
--
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] Add ability to specify partition bounds via jdbc xlang. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #34394:
URL: https://github.com/apache/beam/pull/34394#discussion_r2010704388
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -72,6 +72,8 @@ public Schema configurationSchema() {
// readQuery
.addNullableField("partitionColumn", FieldType.STRING)
.addNullableField("partitions", FieldType.INT16)
+ .addNullableField("longLowerBound", FieldType.INT64)
Review Comment:
We can just support long for now, However this is public API surface and we need to consider extendability. Saying we are going to support other lower/upper bound types in the future, then every time a type added one needs to add a new field to schemaio providers and touch multiple files
--
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