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.
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
Dframe proposal #19
base: master
Are you sure you want to change the base?
Dframe proposal #19
Changes from all commits
a6a8dea
157435f
0bc07f5
c2bbe04
539d5cb
4b2f72b
affe29c
b369015
6de2ece
aedf124
99a7d50
964a9ce
3ec4386
3ffc95a
974c4db
67124b5
8b840c3
a2278f6
40e9cec
ea4d8e7
76f4b62
853ae0a
23a82f4
3d4f163
15d72fa
7741eff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The need for reference counting in Arrow will probably incur some mental overhead and be a source of bugs since it's different from how memory is handled in Go normally. This would probably be a bigger issue in the case of an immutable dataframe than a mutable but it will always be present (right?).
I like the idea of a transaction, chained calls or not, immutable or not. Your suggestion would probably give the greatest flexibility, at the cost of some noise.
This is an alternative which removes some noise (function declaration and return) in client code at the cost of sligthly less flexibility.
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.
yeah... I tried at some point to get rid of it in Go Arrow, but I didn't find a way to support GPGPU (or any alternate memory backing store) and drop this ref-counting thing.
hiding the ref-counting behind the
dframe.Tx
transaction was my way to handle this (so one gets only 1dframe.Frame
that needs to bedefer
ed withRelease()
, like the usualos.File.Close
. This greatly reduces the amount of possible code paths that can go wrong.)that's an interesting alternative. thanks for bringing it up!
it indeed reduces the amount of user-code "noise".
it creates and returns one closure/function per operation, compared to one big closure/function.
I must say I don't know whether that really matters performance wise (CPU, Mem) though...
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 would assume it should not matter performance wise for any but the smallest dataframes. But one should not assume things. :-)
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.
Would it make sense to have a version that accepts a
Reader
instance here? Or, to simply make thesrc
type more liberal (an empty interface under the hood I suppose...)?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 am thinking about having a
dfcsv
package where some appropriate function would take theio.Reader
:and some
database/sql
-like driver registration code so one could write: