-
Notifications
You must be signed in to change notification settings - Fork 125
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
fix: Arrow library usage and memory usage cleanup #769
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
While i needed to upgrade to Arrow v11 to do the cleanup and fixes, #764 should get merged first as he went further than I did and also updated other dependencies and removed the I'm going to second @kenshaw's request to just eliminate the vendoring altogether and just follow the standard Go module handling to make upgrades like this easier in the future. |
@zeroshade nice! I was just looking at what the status of arrow in this driver and saw this PR. Anything we can help here or we can use this branch? |
@yevgenypats if you have any contacts at snowflake, try to push them to review and pay attention to this PR and the linked #764 PR that's the best help I can get right now. |
@zeroshade Thanks for the PR. This PR has many files changes mainly for Arrow library, we will take a look before merging it. |
Unfortunately don't have any contact. @sfc-gh-igarish - this just touches the |
@zeroshade Just an FYI on the status of this driver, we are using this driver in production for a while now for our snowflake destination but I had quite a bit of issues due to a lot of limitations on what it can or cannot do. we ended up just using load to snowflake via CSV and using this driver just to run the |
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. As mentioned in the PR, merge it after #764 merge.
I approved this also. |
Please resolve the conflicts then I can help to merge this PR. I already merged 764. |
Thanks much @sfc-gh-igarish! I'll resolve the conflicts first thing tomorrow for myself and tag you in a comment when I do. |
c8f8d34
to
7da1c7f
Compare
@sfc-gh-igarish rebased and conflicts fixed! thanks! |
Created a linked PR and start running test. |
Description
Updates to Arrow v11 and leverages the compute functions + significantly cleaning up the usage of the Arrow library to be cleaner and fixing the memory leaks that existed in the incorrect usage of the Arrow library. This includes properly calling
Release
on record batches and arrays and addingCheckedAllocator
s to some tests to ensure there are no leaks.This also unifies the Arrow memory allocator used so that consumers can provide their own Arrow
memory.Allocator
if they want.Checklist
make fmt
to fix inconsistent formatsmake lint
to get lint errors and fix all of them