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

Change ScalarValue::List to store ArrayRef #7352

Closed
tustvold opened this issue Aug 21, 2023 · 9 comments · Fixed by #7629
Closed

Change ScalarValue::List to store ArrayRef #7352

tustvold opened this issue Aug 21, 2023 · 9 comments · Fixed by #7629
Assignees
Labels
enhancement New feature or request

Comments

@tustvold
Copy link
Contributor

tustvold commented Aug 21, 2023

Is your feature request related to a problem or challenge?

Currently ScalarValue::List stores a Vec<Option<ScalarValue>>. This is incredibly inefficient as it stores nullability and type information per nested value. It also makes the type very cumbersome to use, as the data cannot easily be fed into the normal array kernels.

Describe the solution you'd like

I would like to change the type of ScalarValue::List to store ArrayRef

Describe alternatives you've considered

No response

Additional context

See #7353

@Dandandan
Copy link
Contributor

Dandandan commented Aug 21, 2023

#996 seems to be a similar suggestion

@Dandandan
Copy link
Contributor

Also see discussions here:

#6837 (comment)
#6837 (comment)

@tustvold
Copy link
Contributor Author

#996 seems to be a similar suggestion

Happy to close this in favor if you prefer, mainly just hoping to bring more attention to this. I know there is a decent amount of ongoing work relating to lists at the moment

@alamb
Copy link
Contributor

alamb commented Aug 21, 2023

I think Change ScalarValue::List to store ArrayRef is a great idea

An ArrayRef is a pretty high overhead way of representing other scalar types but for lists it is different

I also think doing this would allow us to work on / improve the ergonomics of using ListArray in general and set some patterns (like using take and manipulating the list indexes themselves) as well

So 👍 from me

@jayzhan211
Copy link
Contributor

Is anyone working on this? If not, I would like to help with this.

@Dandandan
Copy link
Contributor

Is anyone working on this? If not, I would like to help with this.

I don't think so. I see @tustvold already assigned you on this.

@jayzhan211

This comment was marked as outdated.

@tustvold
Copy link
Contributor Author

we do not need to guarantee the ScalarValue and Array mapping is one-to-one

I'm not sure I follow what you're suggesting but this seems like a fairly fundamental property

@alamb
Copy link
Contributor

alamb commented Oct 13, 2023

FYI I think the PR from @jayzhan211, #7629, is ready to merge. Would anyone else on this chain like a chance to reivew it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants