-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-34334: [Go][CSV] Support list fields #34343
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
|
6fe8583
to
1af26f1
Compare
@zeroshade I've updated this PR to only include list support (and updated per review) as figured out it will be easier to do extensions in a follow-up PR potentially after #34454. |
for _, str := range items { | ||
r.initFieldConverter(valueBldr)(str) | ||
} |
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.
a future enhancement might be to cache this somehow... but it's not needed for this PR
@yevgenypats This looks good to me other than my last two comments. Please add a couple tests for checking error cases and an empty list case. After that this LGTM 😄 thanks again for this! |
Thanks @zeroshade ! BTW - any idea why the |
@yevgenypats it is unrelated to this PR, I'm addressing it in #34488 |
go/arrow/csv/reader.go
Outdated
str = strings.TrimPrefix(str, "{") | ||
str = strings.TrimSuffix(str, "}") |
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.
I think this can be condensed into a single call to strings.Trim(str, "{}")
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.
I can change this but I feel the explicit way is clearer and potentially more efficient. But let me know if you want it the other way.
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.
Looking at the implementations from the strings
package, I'm pretty sure that calling strings.Trim
is more efficient than calling TrimPrefix
and TrimSuffix
separately. At minimum it reduces the number of code branches that get used. Personally I'd prefer using strings.Trim
over the separate calls to prefix/suffix.
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
If you rebase in from master, that'll pick up the fix for the failing macos tests. Once you add a test for the error case, I'm happy to merge this :) |
d383a02
to
a01bc18
Compare
Awesome. Done! |
@yevgenypats Turns out there was a typo in the go workflow yaml in the fix for the cgo macos tests, please rebase main again to get the Go workflows running again (we fixed the typo) thanks! |
a01bc18
to
160aea3
Compare
@zeroshade done! I think this should be ready to go 🚢 |
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.
LGTM
Thanks much!
Benchmark runs are scheduled for baseline = 88d39d5 and contender = 2f3f41f. 2f3f41f is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
This PR only handles