You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/10/28 22:48:23 UTC
[GitHub] [beam] pabloem opened a new pull request #15832: Changing readWithPartitions to work with longs. Removing untested (non-working) code path
pabloem opened a new pull request #15832:
URL: https://github.com/apache/beam/pull/15832
r: @aromanenko-dev
fyi: @chamikaramj @daria-malkova
A small improvement for this transform will be to allow ranges to span a longer space. This is a breaking change, but the transform is relatively new - and it's an easy change to handle - so I'd love to get this in.
If we don't want to introduce a breaking change, I can add API methods for `int` while using `long` internally.
I'm also removing the code under `refineBounds`, because this code will not work properly (it tries to read the table bounds at pipeline construction time - but Beam pipelines run deferred in a remote environment)
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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](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://beam.apache.org/contribute/#make-reviewers-job-easier).
`ValidatesRunner` compliance status (on master branch)
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Samza/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/badge/icon?subject=V1+Streaming">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon?subject=V1+Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/badge/icon?subject=V2+Streaming">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon?subject=Java+8">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon?subject=Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon?subject=Portable+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/badge/icon?subject=Portable">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon?subject=Structured+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon?subject=ValCont">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Samza/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Samza/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
</tbody>
</table>
Examples testing status on various runners
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/badge/icon?subject=V1+Java11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
Post-Commit SDK/Transform Integration Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>Go</th>
<th>Java</th>
<th>Python</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon?subject=3.6">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon?subject=3.7">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon?subject=3.8">
</a>
</td>
</tr>
</tbody>
</table>
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>---</th>
<th>Java</th>
<th>Python</th>
<th>Go</th>
<th>Website</th>
<th>Whitespace</th>
<th>Typescript</th>
</tr>
</thead>
<tbody>
<tr>
<td>Non-portable</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon?subject=Tests">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon?subject=Lint">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon?subject=Docker">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon?subject=Docs">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Portable</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
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)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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] aromanenko-dev commented on a change in pull request #15832: Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r739344376
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
Oh, I see, seems it won'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
[GitHub] [beam] pabloem merged pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem merged pull request #15832:
URL: https://github.com/apache/beam/pull/15832
--
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] aromanenko-dev commented on a change in pull request #15832: Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r739328198
##########
File path: CHANGES.md
##########
@@ -60,6 +60,9 @@
## I/Os
* Support for X source added (Java/Python) ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`. This is a relatively minor
Review comment:
Perhaps, it will be better to move or mention it on `Breaking Changes` section?
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
- refineBounds(input);
- }
-
- int stride = (getUpperBound() - getLowerBound()) / getNumPartitions();
- PCollection<List<Integer>> params =
+ PCollection<KV<Integer, KV<Long, Long>>> params =
input.apply(
Create.of(
Collections.singletonList(
- Arrays.asList(getLowerBound(), getUpperBound(), getNumPartitions()))));
- PCollection<KV<String, Iterable<Integer>>> ranges =
+ KV.of(getNumPartitions(), KV.of(getLowerBound(), getUpperBound())))));
Review comment:
Moving to KVs - is it just a refactoring?
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
Why this code was removed?
--
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] aromanenko-dev commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r739398878
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
What's about this check then? E.g. if upperBound or lowerBound wasn't specified?
--
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] aromanenko-dev commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r741112568
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
Sounds good to me. I think it would be useful to add an assert that it requires to specify the upper and lower bounds if `readWithPartitions()` is used.
--
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] aromanenko-dev commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r741112568
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
Sounds good to me. I think it would be useful to add an assert that it requires to specify the upper and lower bounds if `readWithPartitions()` is used.
##########
File path: CHANGES.md
##########
@@ -70,6 +70,11 @@
* X behavior was changed ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
* A new URN convention was adopted for cross-language transforms and existing URNs were updated. This may break advanced use-cases, for example, if a custom expansion service is used to connect diffrent Beam Java and Python versions. ([BEAM-12047](https://issues.apache.org/jira/browse/BEAM-12047)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`
+ [BEAM-13149](https://issues.apache.org/jira/browse/BEAM-13149). This is a relatively minor
+ breaking change, which we're implementing to improve the usability of the transform without increasing cruft.
+ This transform is relatively new, so we may implement other breaking changes in the future to improve its usability.
+>>>>>>> a24811cef9 (Documenting change as a breaking change)
Review comment:
nit: This line seems was added by chance?
--
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] pabloem commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r740629497
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
in this case, the default values will be used. We could change the parameter to be `Long` instead of `long` and fail with nulls. WDYT?
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
done
##########
File path: CHANGES.md
##########
@@ -70,6 +70,11 @@
* X behavior was changed ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
* A new URN convention was adopted for cross-language transforms and existing URNs were updated. This may break advanced use-cases, for example, if a custom expansion service is used to connect diffrent Beam Java and Python versions. ([BEAM-12047](https://issues.apache.org/jira/browse/BEAM-12047)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`
+ [BEAM-13149](https://issues.apache.org/jira/browse/BEAM-13149). This is a relatively minor
+ breaking change, which we're implementing to improve the usability of the transform without increasing cruft.
+ This transform is relatively new, so we may implement other breaking changes in the future to improve its usability.
+>>>>>>> a24811cef9 (Documenting change as a breaking change)
Review comment:
fixed
--
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] aromanenko-dev commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r741118529
##########
File path: CHANGES.md
##########
@@ -70,6 +70,11 @@
* X behavior was changed ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
* A new URN convention was adopted for cross-language transforms and existing URNs were updated. This may break advanced use-cases, for example, if a custom expansion service is used to connect diffrent Beam Java and Python versions. ([BEAM-12047](https://issues.apache.org/jira/browse/BEAM-12047)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`
+ [BEAM-13149](https://issues.apache.org/jira/browse/BEAM-13149). This is a relatively minor
+ breaking change, which we're implementing to improve the usability of the transform without increasing cruft.
+ This transform is relatively new, so we may implement other breaking changes in the future to improve its usability.
+>>>>>>> a24811cef9 (Documenting change as a breaking change)
Review comment:
nit: This line seems was added by chance?
--
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] pabloem commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r740629497
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
in this case, the default values will be used. We could change the parameter to be `Long` instead of `long` and fail with nulls. WDYT?
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
done
##########
File path: CHANGES.md
##########
@@ -70,6 +70,11 @@
* X behavior was changed ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
* A new URN convention was adopted for cross-language transforms and existing URNs were updated. This may break advanced use-cases, for example, if a custom expansion service is used to connect diffrent Beam Java and Python versions. ([BEAM-12047](https://issues.apache.org/jira/browse/BEAM-12047)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`
+ [BEAM-13149](https://issues.apache.org/jira/browse/BEAM-13149). This is a relatively minor
+ breaking change, which we're implementing to improve the usability of the transform without increasing cruft.
+ This transform is relatively new, so we may implement other breaking changes in the future to improve its usability.
+>>>>>>> a24811cef9 (Documenting change as a breaking change)
Review comment:
fixed
--
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] pabloem commented on pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #15832:
URL: https://github.com/apache/beam/pull/15832#issuecomment-957965488
--
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] pabloem commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r740629497
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
in this case, the default values will be used. We could change the parameter to be `Long` instead of `long` and fail with nulls. WDYT?
--
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] pabloem commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r739396260
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
that's right - this code makes the assumption that *part* of the pipeline will run locally and immediately - and modify local variables.
--
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] pabloem commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r741312088
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
done
##########
File path: CHANGES.md
##########
@@ -70,6 +70,11 @@
* X behavior was changed ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
* A new URN convention was adopted for cross-language transforms and existing URNs were updated. This may break advanced use-cases, for example, if a custom expansion service is used to connect diffrent Beam Java and Python versions. ([BEAM-12047](https://issues.apache.org/jira/browse/BEAM-12047)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`
+ [BEAM-13149](https://issues.apache.org/jira/browse/BEAM-13149). This is a relatively minor
+ breaking change, which we're implementing to improve the usability of the transform without increasing cruft.
+ This transform is relatively new, so we may implement other breaking changes in the future to improve its usability.
+>>>>>>> a24811cef9 (Documenting change as a breaking change)
Review comment:
fixed
--
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] aromanenko-dev commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r741112568
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
Review comment:
Sounds good to me. I think it would be useful to add an assert that it requires to specify the upper and lower bounds if `readWithPartitions()` is used.
##########
File path: CHANGES.md
##########
@@ -70,6 +70,11 @@
* X behavior was changed ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
* A new URN convention was adopted for cross-language transforms and existing URNs were updated. This may break advanced use-cases, for example, if a custom expansion service is used to connect diffrent Beam Java and Python versions. ([BEAM-12047](https://issues.apache.org/jira/browse/BEAM-12047)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`
+ [BEAM-13149](https://issues.apache.org/jira/browse/BEAM-13149). This is a relatively minor
+ breaking change, which we're implementing to improve the usability of the transform without increasing cruft.
+ This transform is relatively new, so we may implement other breaking changes in the future to improve its usability.
+>>>>>>> a24811cef9 (Documenting change as a breaking change)
Review comment:
nit: This line seems was added by chance?
--
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] pabloem commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r739395488
##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1060,53 +1060,36 @@ public void populateDisplayData(DisplayData.Builder builder) {
getUpperBound() - getLowerBound() >= getNumPartitions(),
"The specified number of partitions is more than the difference between upper bound and lower bound");
- if (getUpperBound() == MAX_VALUE || getLowerBound() == 0) {
- refineBounds(input);
- }
-
- int stride = (getUpperBound() - getLowerBound()) / getNumPartitions();
- PCollection<List<Integer>> params =
+ PCollection<KV<Integer, KV<Long, Long>>> params =
input.apply(
Create.of(
Collections.singletonList(
- Arrays.asList(getLowerBound(), getUpperBound(), getNumPartitions()))));
- PCollection<KV<String, Iterable<Integer>>> ranges =
+ KV.of(getNumPartitions(), KV.of(getLowerBound(), getUpperBound())))));
Review comment:
I changed this to KVs because I left the `numPartitions` variable as Integer. This used to be represented as a list, but the list had to have a single type (all ints or all longs) - so that's why I changed it to KV<Integer, KV<Long, Long>> to represent (numPartitions, (Start, End))
--
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] pabloem commented on a change in pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #15832:
URL: https://github.com/apache/beam/pull/15832#discussion_r739397347
##########
File path: CHANGES.md
##########
@@ -60,6 +60,9 @@
## I/Os
* Support for X source added (Java/Python) ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)).
+* We changed the data type for ranges in `JdbcIO.readWithPartitions` from `int` to `long`. This is a relatively minor
Review comment:
Done. Thanks!
--
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] pabloem commented on pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #15832:
URL: https://github.com/apache/beam/pull/15832#issuecomment-957965488
Run Java 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] pabloem commented on pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #15832:
URL: https://github.com/apache/beam/pull/15832#issuecomment-957965488
Run Java 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] pabloem commented on pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #15832:
URL: https://github.com/apache/beam/pull/15832#issuecomment-958005102
Thanks @aromanenko-dev !
--
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] pabloem merged pull request #15832: [BEAM-13149] Changing readWithPartitions to work with longs. Removing untested (non-working) code path
Posted by GitBox <gi...@apache.org>.
pabloem merged pull request #15832:
URL: https://github.com/apache/beam/pull/15832
--
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