-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
ARROW-17107: [Java] Fix variable-width vectors in integration JSON writer #13676
Conversation
lidavidm
commented
Jul 21, 2022
- Clarify that this is meant to be an internal format
- Fix handling of empty variable-width vectors
@@ -83,8 +84,10 @@ | |||
import com.fasterxml.jackson.databind.MappingJsonFactory; | |||
|
|||
/** | |||
* A writer that converts binary Vectors into a JSON format suitable | |||
* A writer that converts binary Vectors into an <em>internal, unstable</em> JSON format suitable |
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.
If this is intended for integration testing, could it be under the test directory?
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.
Integration tests are run via a binary, not via JUnit, so they need to end up in something that gets compiled into a JAR. (Also I'd be a little hesitant to break compatibility immediately here.)
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.
One question, which I assume you've already considered. Otherwise looks good. Thank you for all the added testing.
Makes perfect sense. Thanks
…On Thu, Jul 21, 2022 at 1:20 PM David Li ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileWriter.java
<#13676 (comment)>:
> @@ -83,8 +84,10 @@
import com.fasterxml.jackson.databind.MappingJsonFactory;
/**
- * A writer that converts binary Vectors into a JSON format suitable
+ * A writer that converts binary Vectors into an <em>internal, unstable</em> JSON format suitable
Integration tests are run via a binary, not via JUnit, so they need to end
up in something that gets compiled into a JAR. (Also I'd be a little
hesitant to break compatibility immediately here.)
—
Reply to this email directly, view it on GitHub
<#13676 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2FPAXB467MABONC7CGBW3VVGBFHANCNFSM54HXGXTA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Benchmark runs are scheduled for baseline = d584b8d and contender = c2eda76. c2eda76 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |