You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/13 11:40:26 UTC
[GitHub] [arrow-datafusion] comphead opened a new pull request, #2523: unbox scalars
comphead opened a new pull request, #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #2449.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r875035905
##########
datafusion/core/src/scalar.rs:
##########
@@ -643,11 +643,11 @@ mod tests {
// For different data type, `partial_cmp` returns None.
assert_eq!(
List(
- Some(Box::new(vec![Int64(Some(1)), Int64(Some(5))])),
- Box::new(DataType::Int64),
+ Some(vec![Int64(Some(1)), Int64(Some(5))]),
+ Box::new(DataType::Int32),
Review Comment:
Why the change in type?
##########
datafusion/common/src/scalar.rs:
##########
@@ -934,16 +928,20 @@ impl ScalarValue {
match values {
Some(values) => {
// Push value for each field
- for c in 0..columns.len() {
- let column = columns.get_mut(c).unwrap();
- column.push(values[c].clone());
+ for (i, v) in
Review Comment:
A perhaps more idiomatic way to write this would be something like
```
for (column, value) in columns.iter_mut().zip(values)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r874144292
##########
datafusion/common/src/scalar.rs:
##########
@@ -934,16 +928,20 @@ impl ScalarValue {
match values {
Some(values) => {
// Push value for each field
- for c in 0..columns.len() {
- let column = columns.get_mut(c).unwrap();
- column.push(values[c].clone());
+ for (i, v) in
Review Comment:
👍 this is a nicer implementation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
comphead commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r875053753
##########
datafusion/common/src/scalar.rs:
##########
@@ -934,16 +928,20 @@ impl ScalarValue {
match values {
Some(values) => {
// Push value for each field
- for c in 0..columns.len() {
- let column = columns.get_mut(c).unwrap();
- column.push(values[c].clone());
+ for (i, v) in
Review Comment:
Yes, thats more concise. Fixed in 2 places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1127584569
> @alamb What do you think about Box<[Field]>?
It seems fine to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1127797887
If you prefer `Box<Vec<Field>>` over `Box<[Field]>`, I don't feel strongly. The latter avoids a pointer indirection, and associated memory allocation, but the former is perhaps more consistent with rest of the enumeration (which uses Vec despite not actually needing to).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1126367412
So I got this to not change the size by making
```
List(Option<Vec<ScalarValue>>, Box<DataType>),
Struct(Option<Vec<ScalarValue>>, Box<[Field]>),
```
@alamb What do you think about `Box<[Field]>`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1129188564
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead closed pull request #2523: unbox scalars
Posted by GitBox <gi...@apache.org>.
comphead closed pull request #2523: unbox scalars
URL: https://github.com/apache/arrow-datafusion/pull/2523
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1128122521
I'll wait until tomorrow to see if @tustvold has any more comments, and if not I'll merge it in
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold merged pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r872692149
##########
datafusion/common/src/scalar.rs:
##########
@@ -74,8 +74,7 @@ pub enum ScalarValue {
/// large binary
LargeBinary(Option<Vec<u8>>),
/// list of nested ScalarValue (boxed to reduce size_of(ScalarValue))
- #[allow(clippy::box_collection)]
- List(Option<Box<Vec<ScalarValue>>>, Box<DataType>),
+ List(Option<Vec<ScalarValue>>, DataType),
Review Comment:
I'm not sure about unboxing DataType here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r872660380
##########
datafusion/common/src/scalar.rs:
##########
@@ -74,8 +74,7 @@ pub enum ScalarValue {
/// large binary
LargeBinary(Option<Vec<u8>>),
/// list of nested ScalarValue (boxed to reduce size_of(ScalarValue))
Review Comment:
```suggestion
/// list of nested ScalarValue
```
##########
datafusion/common/src/scalar.rs:
##########
@@ -95,8 +94,7 @@ pub enum ScalarValue {
/// Interval with MonthDayNano unit
IntervalMonthDayNano(Option<i128>),
/// struct of nested ScalarValue (boxed to reduce size_of(ScalarValue))
Review Comment:
```suggestion
/// struct of nested ScalarValue
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
comphead commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r875052977
##########
datafusion/core/src/scalar.rs:
##########
@@ -643,11 +643,11 @@ mod tests {
// For different data type, `partial_cmp` returns None.
assert_eq!(
List(
- Some(Box::new(vec![Int64(Some(1)), Int64(Some(5))])),
- Box::new(DataType::Int64),
+ Some(vec![Int64(Some(1)), Int64(Some(5))]),
+ Box::new(DataType::Int32),
Review Comment:
fixed. Thanks for finding that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
comphead commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1127739566
so we going with changes below?
```
List(Option<Vec<ScalarValue>>, Box<DataType>),
Struct(Option<Vec<ScalarValue>>, Box<Vec<Field>>),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
comphead commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r874146979
##########
datafusion/common/src/scalar.rs:
##########
@@ -934,16 +928,20 @@ impl ScalarValue {
match values {
Some(values) => {
// Push value for each field
- for c in 0..columns.len() {
- let column = columns.get_mut(c).unwrap();
- column.push(values[c].clone());
+ for (i, v) in
Review Comment:
clippy complained on`for c in 0..columns.len()` :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
comphead commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r874146979
##########
datafusion/common/src/scalar.rs:
##########
@@ -934,16 +928,20 @@ impl ScalarValue {
match values {
Some(values) => {
// Push value for each field
- for c in 0..columns.len() {
- let column = columns.get_mut(c).unwrap();
- column.push(values[c].clone());
+ for (i, v) in
Review Comment:
clippy complained on that :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
comphead commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1128033309
@tustvold I added boxing back.
For now its
```
Struct(Option<Vec<ScalarValue>>, Box<Vec<Field>>),
```
as its more consistent, you are right. If you feel we should go to Box<[Field]> let me know
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
comphead commented on PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#issuecomment-1126886258
Thanks @alamb @tustvold for the feedback, please let me know the final decision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2523: Fix Redundant ScalarValue Boxed Collection
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2523:
URL: https://github.com/apache/arrow-datafusion/pull/2523#discussion_r874152569
##########
datafusion/common/src/scalar.rs:
##########
@@ -934,16 +928,20 @@ impl ScalarValue {
match values {
Some(values) => {
// Push value for each field
- for c in 0..columns.len() {
- let column = columns.get_mut(c).unwrap();
- column.push(values[c].clone());
+ for (i, v) in
Review Comment:
All the better that removing a layer of `Box`ing lets clippy fix things more easily 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org