Skip to content
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

nit : Remove checked allocator in arrow code usages #6341

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

k-anshul
Copy link
Member

@k-anshul k-anshul commented Jan 2, 2025

The CheckedAllocator is expected to be used in test code only. Mostly adds extra overheads as well.

@k-anshul k-anshul self-assigned this Jan 2, 2025
@@ -160,6 +160,7 @@ func (r *sqlResolver) generalExport(ctx context.Context, w io.Writer, filename s
if err != nil {
return err
}
defer res.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I don't think the sqlResolver is currently used for exporting anywhere, so it's probably not the cause of memory leaks. But there may be other places – maybe we should generally search for calls to OLAPStore.Execute and audit that results are closed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it's probably not the cause of memory leaks

Yeah.

maybe we should generally search for calls to OLAPStore.Execute and audit that results are closed

Yeah that's how I found this one as well.

@begelundmuller begelundmuller merged commit 54500f6 into main Jan 6, 2025
9 checks passed
@begelundmuller begelundmuller deleted the remove_checked_allocator branch January 6, 2025 10:01
k-anshul added a commit that referenced this pull request Jan 6, 2025
* close results in exports in sql resolver

* remove memory.CheckedAllocator from arrow code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants