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 2022/02/15 11:24:02 UTC
[GitHub] [beam] iemejia opened a new pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
iemejia opened a new pull request #16856:
URL: https://github.com/apache/beam/pull/16856
**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:
- [ ] [**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_PythonUsingJava_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_PythonUsingJava_Dataflow/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_PythonUsingJavaSQL_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_PythonUsingJavaSQL_Dataflow/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_JavaUsingPython_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_JavaUsingPython_Dataflow/lastCompletedBuild/badge/icon">
</a><br>
</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] RyanSkraba commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r806931622
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -33,21 +37,30 @@ public static CountIfFn combineFn() {
public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @AutoValue
+ public abstract static class Accum implements Serializable {
+ abstract boolean isExpressionFalse();
Review comment:
Hello! If I'm reading this correctly and Accum is not used for other reasons outside of the class, this is strictly tied to whether countIfResult is zero... This might be an opportunity to simplify the code!
--
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] iemejia merged pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
iemejia merged pull request #16856:
URL: https://github.com/apache/beam/pull/16856
--
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] iemejia commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
iemejia commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r808129562
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
+ private final Combine.CombineFn<Boolean, long[], Long> countFn =
+ (Combine.CombineFn<Boolean, long[], Long>) Count.<Boolean>combineFn();
Review comment:
Agree, I hesitated too make it public and make the `CountIfFn` inherit it and change just to override the addInput method, what let me puzzled was how to make `CompineFn` composable so I could just 'filter' and then apply `CountFn`
--
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] iemejia commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
iemejia commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r807100993
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -33,21 +37,30 @@ public static CountIfFn combineFn() {
public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @AutoValue
+ public abstract static class Accum implements Serializable {
+ abstract boolean isExpressionFalse();
Review comment:
Oh you have a point actually CountIf is a specific case of Count I am going to reuse Count's logic then. Thanks for pointing this out!
--
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] RyanSkraba commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r806931622
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -33,21 +37,30 @@ public static CountIfFn combineFn() {
public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @AutoValue
+ public abstract static class Accum implements Serializable {
+ abstract boolean isExpressionFalse();
Review comment:
Hello! If I'm reading this correctly and Accum is not used for other reasons outside of the class, this is strictly tied to whether countIfResult is zero... This might be an opportunity to simplify the code!
--
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] iemejia commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
iemejia commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r807100993
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -33,21 +37,30 @@ public static CountIfFn combineFn() {
public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @AutoValue
+ public abstract static class Accum implements Serializable {
+ abstract boolean isExpressionFalse();
Review comment:
Oh you have a point actually CountIf is a specific case of Count I am going to reuse Count's logic then. Thanks for pointing this out!
--
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] RyanSkraba commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r807730968
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -31,62 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
-
- @AutoValue
- public abstract static class Accum implements Serializable {
- abstract boolean isExpressionFalse();
-
- abstract long countIfResult();
-
- static Accum empty() {
- return of(true, 0L);
- }
-
- static Accum of(boolean isExpressionFalse, long countIfResult) {
- return new AutoValue_CountIf_CountIfFn_Accum(isExpressionFalse, countIfResult);
- }
- }
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
Review comment:
Nice cleanup! At this point, would it be worthwhile to let Count.CountFn be public, so we could just inherit it? I don't really have a strong 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
[GitHub] [beam] iemejia commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
iemejia commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r808121567
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
Review comment:
Yes, I hesitated but this is internal and this class is only instantiated by SQL when it registers the built-in aggregators so we should be good. sql/impl is not explicitly marked as Internal but I agree that it should.
--
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] kennknowles commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r807976012
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
Review comment:
I suppose technically this is a breaking change. But of course everything implementing SQL is not intended for users. Is `sql/impl` marked `@Internal`? (this change still LGTM because it is not actually intended as a user API)
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
+ private final Combine.CombineFn<Boolean, long[], Long> countFn =
+ (Combine.CombineFn<Boolean, long[], Long>) Count.<Boolean>combineFn();
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @Override
+ public long[] createAccumulator() {
+ return countFn.createAccumulator();
}
@Override
- public Accum createAccumulator() {
- return new Accum();
+ public long[] addInput(long[] accumulator, Boolean input) {
+ if (Boolean.TRUE.equals(input)) {
+ countFn.addInput(accumulator, input);
+ }
+ return accumulator;
}
@Override
- public Accum addInput(Accum accum, Boolean input) {
- if (input) {
- accum.isExpressionFalse = false;
Review comment:
I never looked at this code before. Now that I see it... why was this field ever needed? Seems like the `0L` result contains all the useful info. Makes me worried there is something tricky that I am not noticing...
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
+ private final Combine.CombineFn<Boolean, long[], Long> countFn =
+ (Combine.CombineFn<Boolean, long[], Long>) Count.<Boolean>combineFn();
Review comment:
This cast makes me think that `Count.combineFn()` worked too hard to hide the accumulator type. It should just expose it.
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
+ private final Combine.CombineFn<Boolean, long[], Long> countFn =
+ (Combine.CombineFn<Boolean, long[], Long>) Count.<Boolean>combineFn();
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @Override
+ public long[] createAccumulator() {
+ return countFn.createAccumulator();
}
@Override
- public Accum createAccumulator() {
- return new Accum();
+ public long[] addInput(long[] accumulator, Boolean input) {
+ if (Boolean.TRUE.equals(input)) {
+ countFn.addInput(accumulator, input);
+ }
+ return accumulator;
}
@Override
- public Accum addInput(Accum accum, Boolean input) {
- if (input) {
- accum.isExpressionFalse = false;
- accum.countIfResult += 1;
- }
- return accum;
+ public long[] mergeAccumulators(Iterable<long[]> accumulators) {
+ return countFn.mergeAccumulators(accumulators);
}
@Override
- public Accum mergeAccumulators(Iterable<Accum> accums) {
- CountIfFn.Accum merged = createAccumulator();
- for (CountIfFn.Accum accum : accums) {
- if (!accum.isExpressionFalse) {
- merged.isExpressionFalse = false;
- merged.countIfResult += accum.countIfResult;
- }
- }
- return merged;
+ public Long extractOutput(long[] accumulator) {
+ return countFn.extractOutput(accumulator);
}
@Override
- public Long extractOutput(Accum accum) {
- if (!accum.isExpressionFalse) {
- return accum.countIfResult;
- }
- return 0L;
+ public Coder<long[]> getAccumulatorCoder(CoderRegistry registry, Coder<Boolean> inputCoder)
+ throws CannotProvideCoderException {
+ return countFn.getAccumulatorCoder(registry, inputCoder);
Review comment:
Technically changing coders here would break in-place update. But SQL really just cannot be relied on for that, since the optimizer might change. So I am just noting that I _explicitly_ say it is OK to break in-place update here.
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -31,62 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
-
- @AutoValue
- public abstract static class Accum implements Serializable {
- abstract boolean isExpressionFalse();
-
- abstract long countIfResult();
-
- static Accum empty() {
- return of(true, 0L);
- }
-
- static Accum of(boolean isExpressionFalse, long countIfResult) {
- return new AutoValue_CountIf_CountIfFn_Accum(isExpressionFalse, countIfResult);
- }
- }
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
Review comment:
IMO using it via composition like this is preferable anyhow. You can see it is more flexible, since you can achieve all the same things without needing it to be public.
--
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] iemejia commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
iemejia commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r808123618
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
+ private final Combine.CombineFn<Boolean, long[], Long> countFn =
+ (Combine.CombineFn<Boolean, long[], Long>) Count.<Boolean>combineFn();
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @Override
+ public long[] createAccumulator() {
+ return countFn.createAccumulator();
}
@Override
- public Accum createAccumulator() {
- return new Accum();
+ public long[] addInput(long[] accumulator, Boolean input) {
+ if (Boolean.TRUE.equals(input)) {
+ countFn.addInput(accumulator, input);
+ }
+ return accumulator;
}
@Override
- public Accum addInput(Accum accum, Boolean input) {
- if (input) {
- accum.isExpressionFalse = false;
Review comment:
It was the same conclusion we arrived too with Ryan, that's why I went into the simplification road. I added the tests to try to find issues and fixed the ones I saw.
--
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] iemejia commented on a change in pull request #16856: [BEAM-13202] Add Coder to CountIfFn.Accum
Posted by GitBox <gi...@apache.org>.
iemejia commented on a change in pull request #16856:
URL: https://github.com/apache/beam/pull/16856#discussion_r808125396
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/CountIf.java
##########
@@ -27,49 +30,41 @@
public class CountIf {
private CountIf() {}
- public static CountIfFn combineFn() {
- return new CountIf.CountIfFn();
+ public static Combine.CombineFn<Boolean, ?, Long> combineFn() {
+ return new CountIfFn();
}
- public static class CountIfFn extends Combine.CombineFn<Boolean, CountIfFn.Accum, Long> {
+ public static class CountIfFn extends Combine.CombineFn<Boolean, long[], Long> {
+ private final Combine.CombineFn<Boolean, long[], Long> countFn =
+ (Combine.CombineFn<Boolean, long[], Long>) Count.<Boolean>combineFn();
- public static class Accum implements Serializable {
- boolean isExpressionFalse = true;
- long countIfResult = 0L;
+ @Override
+ public long[] createAccumulator() {
+ return countFn.createAccumulator();
}
@Override
- public Accum createAccumulator() {
- return new Accum();
+ public long[] addInput(long[] accumulator, Boolean input) {
+ if (Boolean.TRUE.equals(input)) {
+ countFn.addInput(accumulator, input);
+ }
+ return accumulator;
}
@Override
- public Accum addInput(Accum accum, Boolean input) {
- if (input) {
- accum.isExpressionFalse = false;
- accum.countIfResult += 1;
- }
- return accum;
+ public long[] mergeAccumulators(Iterable<long[]> accumulators) {
+ return countFn.mergeAccumulators(accumulators);
}
@Override
- public Accum mergeAccumulators(Iterable<Accum> accums) {
- CountIfFn.Accum merged = createAccumulator();
- for (CountIfFn.Accum accum : accums) {
- if (!accum.isExpressionFalse) {
- merged.isExpressionFalse = false;
- merged.countIfResult += accum.countIfResult;
- }
- }
- return merged;
+ public Long extractOutput(long[] accumulator) {
+ return countFn.extractOutput(accumulator);
}
@Override
- public Long extractOutput(Accum accum) {
- if (!accum.isExpressionFalse) {
- return accum.countIfResult;
- }
- return 0L;
+ public Coder<long[]> getAccumulatorCoder(CoderRegistry registry, Coder<Boolean> inputCoder)
+ throws CannotProvideCoderException {
+ return countFn.getAccumulatorCoder(registry, inputCoder);
Review comment:
Technically we did not have a specified `Coder` so it was breaking when running on a distributed system as the JIRA ticket reported so backwards compatibility seems less of an issue ;)
--
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