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 2020/06/24 19:38:45 UTC
[GitHub] [beam] Imfuyuwei opened a new pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL and adde…
Imfuyuwei opened a new pull request #12079:
URL: https://github.com/apache/beam/pull/12079
R: @amaliujia
CC: @kennknowles
Added support for BIT_AND aggregation function in Beam SQL.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] 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).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark
--- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Imfuyuwei commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
Imfuyuwei commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r445160365
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
Review comment:
Because -1L is represented as 64 bits of 1 in binary while 1L only has one 1 at the least significant bit.
In order to do bit_and operation, I think the initial bit mask should consists of only 1s, so I use -1L.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r445169828
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
Review comment:
O for that pipeline run and then wait. Thanks that's a nice catch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r445163189
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
Review comment:
That's a good point 👍 I should go back to re-visit CMU 15231 course slides.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r445163189
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
Review comment:
That's a good point 👍 I should go back to re-visit CMU 15213 course slides.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Imfuyuwei commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
Imfuyuwei commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r445167391
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
Review comment:
Thanks!
BTW I want to mention that I also added 1 line at the end of the previous testBitOrFunction(). I noticed that without this line, the previous bit_or test will always no matter what expected result I set, which makes it an invalid test.
It will be good if you can take a look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia merged pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
amaliujia merged pull request #12079:
URL: https://github.com/apache/beam/pull/12079
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r446437930
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
+ }
+
+ @Override
+ public Long addInput(Long accum, T input) {
+ return accum & input.longValue();
Review comment:
If consider NULL is unknown, then unknown op something should be unknown.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Imfuyuwei commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
Imfuyuwei commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r445167391
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
Review comment:
Thanks!
BTW I want to mention that I also added 1 line at the end of the previous testBitOrFunction(). I noticed that without this line, the previous bit_or test would always pass no matter what expected result I set, which made it an invalid test.
It will be good if you can take a look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r445151779
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
Review comment:
-1L makes it a bit harder to read. Why not use 1L instead?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12079: [BEAM-9890] Support BIT_AND aggregation function in Beam SQL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12079:
URL: https://github.com/apache/beam/pull/12079#discussion_r446437758
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -383,4 +392,30 @@ public Long extractOutput(Long accum) {
return accum;
}
}
+
+ static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {
+ @Override
+ public Long createAccumulator() {
+ return -1L;
+ }
+
+ @Override
+ public Long addInput(Long accum, T input) {
+ return accum & input.longValue();
Review comment:
In fact, I wasn't sure what's the result of `NULL & long`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org