feat(arrow-csv): support encoding of binary in CSV writer #5782
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allows for writing binary (Binary, LargeBinary, and FixedSizeBinary) to CSV. Note: FixedSizeBinary was already being supported in this way. Values are encoded as HEX, by using the default Arrow formatter. A test was added that accounts for null values when encoding all three binary types in CSV.
|
It is worth noting the discussion here: #3292 where, at the time, I think there was some contention about encoding binary in CSV based on other Arrow implementations (pyarrow). |
hiltontj
changed the title
tustvold
approved these changes
May 20, 2024
There was a problem hiding this comment.
I think this is fine, we can always leave the door open to encoding binary data as base64 (or some other encoding) down the line. Whilst innefficient, hex encoding should avoid the issues in #3292 where CSV doesn't really define how escaping should behave
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allows for writing binary (Binary, LargeBinary, and FixedSizeBinary) to CSV. Note: FixedSizeBinary was already being supported in this way.
Values are encoded as HEX, by using the default Arrow formatter.
A test was added that accounts for null values when encoding all three binary types in CSV.
Which issue does this PR close?
Closes #3921.
Rationale for this change
Currently,
FixedSizeBinaryis encoded in CSV (and JSON) as HEX (see #3291 (comment)). Therefore, it seemed reasonable to lift the restriction that was preventing writingBinaryandLargeBinaryarrays.What changes are included in this PR?
The condition that checks if the datatype on the array is
BinaryorLargeBinarywas removed, and a test was added to check writing to CSV of the three binary types:Binary,LargeBinary, andFixedSizeBinary. The test also checks for null values.Are there any user-facing changes?
This will change the behaviour of the writer for users, but should not be a breaking change, since previously, an error was being thrown when attempting to write these data types to CSV.
We may need to document that such types are encoded as HEX in the CSV writer, as they are with the standard Arrow formatter.