Fix Redundant ScalarValue Boxed Collection #2523

comphead · 2022-05-13T11:40:24Z

Which issue does this PR close?

Closes #2449.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb

Thank you very much @comphead

They key test is that size_of_scalar is not changed

https://github.com/apache/arrow-datafusion/blob/41b4e491663029f653e491b110d0b5e74d08a0b6/datafusion/core/src/scalar.rs#L373-L383

However, when I investigated more it turns out that this test was broken (see #2531)

When I reenabled the test it shows that this PR makes the size of ScalarValue increase from 48 bytes to 64 bytes

cc @tustvold

datafusion/common/src/scalar.rs

tustvold

I think this PR removes a non-redundant Box<DataType>. Is it possible this is the cause of the size change?

tustvold · 2022-05-13T18:49:13Z

datafusion/common/src/scalar.rs

    /// 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),


I'm not sure about unboxing DataType here

tustvold · 2022-05-13T19:15:14Z

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]>?

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

comphead · 2022-05-15T08:30:12Z

Thanks @alamb @tustvold for the feedback, please let me know the final decision on what exactly has to be boxed.

alamb · 2022-05-16T12:04:25Z

@alamb What do you think about Box<[Field]>?

It seems fine to me

comphead · 2022-05-16T14:21:35Z

so we going with changes below?

List(Option<Vec<ScalarValue>>, Box<DataType>),
Struct(Option<Vec<ScalarValue>>, Box<Vec<Field>>),

tustvold · 2022-05-16T15:11:33Z

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).

comphead · 2022-05-16T19:06:55Z

@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

alamb

I think this looks great @comphead -- thank you very much for sticking with it.

I also merged this PR locally with apache/master to get #2531 and verified that the tests still pass 🎉

alamb · 2022-05-16T20:48:31Z

datafusion/common/src/scalar.rs

-                                for c in 0..columns.len() {
-                                    let column = columns.get_mut(c).unwrap();
-                                    column.push(values[c].clone());
+                                for (i, v) in


👍 this is a nicer implementation

clippy complained onfor c in 0..columns.len() :)

All the better that removing a layer of Boxing lets clippy fix things more easily 👍

A perhaps more idiomatic way to write this would be something like

for (column, value) in columns.iter_mut().zip(values)

Yes, thats more concise. Fixed in 2 places.

alamb · 2022-05-16T20:50:56Z

I'll wait until tomorrow to see if @tustvold has any more comments, and if not I'll merge it in

tustvold

Other than a minor nit and a questionable test change, this looks good to me 👍

Thank you, and apologies for the back and forth

tustvold · 2022-05-17T16:33:39Z

datafusion/common/src/scalar.rs

-                                for c in 0..columns.len() {
-                                    let column = columns.get_mut(c).unwrap();
-                                    column.push(values[c].clone());
+                                for (i, v) in


A perhaps more idiomatic way to write this would be something like

for (column, value) in columns.iter_mut().zip(values)

tustvold · 2022-05-17T16:34:00Z

datafusion/core/src/scalar.rs

-                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),


Why the change in type?

fixed. Thanks for finding that

alamb · 2022-05-17T18:29:58Z

👍

comphead and others added 2 commits May 13, 2022 04:34

unbox scalars

95ea4db

Merge branch 'master' into unbox_scalar_merged

d750e43

comphead closed this May 13, 2022

comphead reopened this May 13, 2022

comphead changed the title ~~unbox scalars~~ Fix Redundant ScalarValue Boxed Collection May 13, 2022

github-actions bot added the datafusion label May 13, 2022

comphead added 2 commits May 13, 2022 05:20

fix conflicts

5661512

fix clippy

77b3557

comphead marked this pull request as ready for review May 13, 2022 15:08

alamb mentioned this pull request May 13, 2022

Fix size_of_scalar test #2531

Merged

alamb reviewed May 13, 2022

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

tustvold reviewed May 13, 2022

View reviewed changes

comphead and others added 2 commits May 15, 2022 01:21

Update datafusion/common/src/scalar.rs

1d883ac

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/common/src/scalar.rs

d64f224

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

comphead and others added 2 commits May 16, 2022 09:31

Merge branch 'apache:master' into unbox_scalar_merged

66c8c2f

boxing datatype

f957fc3

alamb approved these changes May 16, 2022

View reviewed changes

tustvold reviewed May 17, 2022

View reviewed changes

fixing test

b9ea1a5

tustvold merged commit c924536 into apache:master May 17, 2022

tustvold added the api change Changes the API exposed to users of the crate label May 17, 2022

comphead deleted the unbox_scalar_merged branch September 8, 2022 23:03

Fix Redundant ScalarValue Boxed Collection #2523

Fix Redundant ScalarValue Boxed Collection #2523

Uh oh!

Conversation

comphead commented May 13, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold commented May 13, 2022

Uh oh!

comphead commented May 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented May 16, 2022

Uh oh!

comphead commented May 16, 2022

Uh oh!

tustvold commented May 16, 2022

Uh oh!

comphead commented May 16, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented May 16, 2022

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

comphead commented May 15, 2022 •

edited

Loading

comphead May 16, 2022 •

edited

Loading