[BEAM-2817] BigQuery queries are allowed to run in either BATCH or IN… #4606
Conversation
|
Closed the previous pull request and opened this one. Thanks |
|
LGTM Thanks for the contribution. For reference, previous review is in #4226. |
|
Run Java PreCommit |
|
Please fix the Javadoc errors. 2018-02-06T20:26:53.975 [ERROR] Exit code: 1 - /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/src/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java:915: error: reference not found https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/17572/console |
|
Sure, thanks |
|
Please let me know when it's ready. |
|
Will do, I'll get to it tonight after work. Do I run Java Precommit to test it out? @chamikaramj |
|
Java PreCommit will automatically run when you push your update. Also, please merge your commits into a single commit. |
…TERACTIVE mode [BEAM-2817] fixed javadoc errors [BEAM-2817] fixed javadoc errors
|
This is ready @chamikaramj |
|
Thanks. Merging. |
|
|
||
| PAssert.that(output) | ||
| .containsInAnyOrder(ImmutableList.of(KV.of("a", 1L), KV.of("b", 2L), KV.of("c", 3L))); | ||
| assertEquals(read.getPriority(), BigQueryIO.TypedRead.Priority.INTERACTIVE); |
There was a problem hiding this comment.
Unfortunately this doesn't actually do anything, and the tests are vacuous (pass without verifying what they are supposed to verify).
read.getPriority() simply returns what you set on .usingInteractivePriority() - you need to be verifying the priority that was used on the actual BigQuery load job.
| super(stepUuid, bqServices, coder, parseFn); | ||
| this.query = checkNotNull(query, "query"); | ||
| this.flattenResults = checkNotNull(flattenResults, "flattenResults"); | ||
| this.useLegacySql = checkNotNull(useLegacySql, "useLegacySql"); | ||
| this.dryRunJobStats = new AtomicReference<>(); | ||
| if (priority != BigQueryIO.TypedRead.Priority.BATCH |
There was a problem hiding this comment.
This condition is always true - "priority != a || priority != b" is true by definition, because it can't equal a and b at the same time. I suppose you probably meant "&&", but then again I'm not sure why this condition is needed at all - why not just "this.priority = priority"?
There was a problem hiding this comment.
Is there a need to rollback before the release ? Looks like this just keeps the previous default, right ?
There was a problem hiding this comment.
I'm fixing this in my PR. Probably not too big of a deal, just this feature won't really be usable in the release, but will be in the next one. No regression.
There was a problem hiding this comment.
Good find. This should be && as this sets the Priority to default. This was done in the case that an incorrect priority is passed in that is neither batch or interactive. Please let me know the next steps as I see that you will fix this part of the PR. I can fix the unit tests if needed.
…TERACTIVE mode
DESCRIPTION HERE
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.