-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-7013] Update BigQueryHllSketchCompatibilityIT to cover empty sketch cases #9778
Conversation
Run Java PostCommit |
See all the 4 test cases pass here: https://builds.apache.org/job/beam_PostCommit_Java_PR/237/testReport/org.apache.beam.sdk.extensions.zetasketch/BigQueryHllSketchCompatibilityIT/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice that you got this to work and that it's now available to folks as an example how to do this!
Main question is double-checking that this is testing what we think it is testing.
Beyond this PR, we should make sure that this gets covered in a pattern. But also: is there any way we could make the conversion functors (parseQueryResultToByteArray, and the inlined one for the other direction) available to users as utility objects or methods? It seems suboptimal if every person who wants to have Beam and BQ interoperate on HLL++ would need to copy out the code from a pattern. E.g., if there's ever a new edge case to be handled in this conversion, it's easier to update that centrally.
...rc/test/java/org/apache/beam/sdk/extensions/zetasketch/BigQueryHllSketchCompatibilityIT.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/beam/sdk/extensions/zetasketch/BigQueryHllSketchCompatibilityIT.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/beam/sdk/extensions/zetasketch/BigQueryHllSketchCompatibilityIT.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/beam/sdk/extensions/zetasketch/BigQueryHllSketchCompatibilityIT.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/beam/sdk/extensions/zetasketch/BigQueryHllSketchCompatibilityIT.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/beam/sdk/extensions/zetasketch/BigQueryHllSketchCompatibilityIT.java
Show resolved
Hide resolved
I have considered this but unfortunately we cannot do that, because in the same function users might want to parse other fields. However I do find a way to extract part of the logic into its own function. See the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay, thank you for adding the utility function to HllCount.java! It's a pity we can't do it for both directions...
.../extensions/zetasketch/src/main/java/org/apache/beam/sdk/extensions/zetasketch/HllCount.java
Outdated
Show resolved
Hide resolved
Run Java PreCommit |
Run Java_Examples_Dataflow PreCommit |
Run Java PostCommit |
r: @zfraa
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.