-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: add ability to write Range values with JSONStreamWriter #2498
Conversation
/gcbrun |
1 similar comment
/gcbrun |
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.
Looks good! Some nits.
ServerStream<ReadRowsResponse> stream = client.readRowsCallable().call(readRowsRequest); | ||
for (ReadRowsResponse response : stream) { | ||
Preconditions.checkState(response.hasArrowRecordBatch()); | ||
rowCount += response.getRowCount(); |
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.
validate the value?
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.
Added validation.
|
||
JSONObject row3 = new JSONObject(); | ||
JSONObject rangeDate3 = new JSONObject(); | ||
rangeDate3.put("start", 18262); |
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.
test date as a string?
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.
Updated to use string values.
...gquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1/JsonToProtoMessageTest.java
Show resolved
Hide resolved
...gquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1/JsonToProtoMessageTest.java
Show resolved
Hide resolved
…eJSONStreamWriter
…eJSONStreamWriter
/gcbrun |
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.
@yirutang
Thanks for the review. I've updated the PR with the suggestions. PTAL.
...gquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1/JsonToProtoMessageTest.java
Show resolved
Hide resolved
|
||
JSONObject row3 = new JSONObject(); | ||
JSONObject rangeDate3 = new JSONObject(); | ||
rangeDate3.put("start", 18262); |
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.
Updated to use string values.
ServerStream<ReadRowsResponse> stream = client.readRowsCallable().call(readRowsRequest); | ||
for (ReadRowsResponse response : stream) { | ||
Preconditions.checkState(response.hasArrowRecordBatch()); | ||
rowCount += response.getRowCount(); |
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.
Added validation.
/gcbrun |
...gquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1/JsonToProtoMessageTest.java
Show resolved
Hide resolved
...uerystorage/src/test/java/com/google/cloud/bigquery/storage/v1/it/ITBigQueryStorageTest.java
Outdated
Show resolved
Hide resolved
/gcbrun |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #2481 ☕️