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

expose get_or_insert_key and some optimization function for dict #6876

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rluvaton
Copy link
Contributor

@rluvaton rluvaton commented Dec 12, 2024

Which issue does this PR close?

None

Rationale for this change

I want to insert to dict without doing value lookup, this allow me to take advantage of some assumption I have about the data

What changes are included in this PR?

-

Are there any user-facing changes?

Yes, there are user facing changed and I think I documented them

TODO

  • add tests

@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 12, 2024
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

I wonder if we're overloading more onto this builder than makes sense? If you're going to use unsafe to bypass the builder checks, you might as well just construct the dictionary array directly?

@rluvaton rluvaton marked this pull request as ready for review December 12, 2024 20:31
@rluvaton
Copy link
Contributor Author

rluvaton commented Dec 12, 2024

Sometimes I can't, I just have a function that gets the builder and need to append to it, also the builder make sure you don't insert duplicate values

@tustvold
Copy link
Contributor

also the builder make sure you don't insert duplicate values

Right but you are just bypassing this with these methods?

I just have a function that gets the builder and need to append to it

And yet you know that it doesn't contain duplicate values? This feels exceedingly niche...

@rluvaton
Copy link
Contributor Author

rluvaton commented Dec 12, 2024

It's not that it doesn't contain duplicate values, it's that I don't insert duplicate values (basically i need to append some indices from other dictionary so this will allow me to do what I did in extend_dictonary but in a more fine grain way

@tustvold
Copy link
Contributor

Perhaps we might take a step back here and file a ticket with a concrete use-case, explaining what it is you're trying to achieve and we can then go from there. As it stands this PR dramatically increases the API surface without clear justification

@tustvold tustvold marked this pull request as draft December 15, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants