This is an automated email from the ASF dual-hosted git repository.
liukun pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow-datafusion.git
The following commit(s) were added to refs/heads/master by this push:
new 15a7d5b2f fix: remove TODOs linked to arrow#3147 (#4540)
15a7d5b2f is described below
commit 15a7d5b2fea335ecd7262170d8187a68cb3f2ba3
Author: Marco Neumann <[email protected]>
AuthorDate: Thu Dec 8 02:13:14 2022 +0000
fix: remove TODOs linked to arrow#3147 (#4540)
Arrow now offers `size` methods for `DataType` and `Field`, so these
TODOs can be fixed.
---
datafusion/common/src/scalar.rs | 12 +++++++-----
.../physical-expr/src/aggregate/approx_percentile_cont.rs | 4 +++-
datafusion/physical-expr/src/aggregate/array_agg.rs | 3 ++-
datafusion/physical-expr/src/aggregate/array_agg_distinct.rs | 3 ++-
datafusion/physical-expr/src/aggregate/count_distinct.rs | 8 +++++++-
datafusion/physical-expr/src/aggregate/median.rs | 3 ++-
datafusion/physical-expr/src/aggregate/sum_distinct.rs | 3 ++-
7 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/datafusion/common/src/scalar.rs b/datafusion/common/src/scalar.rs
index 46a1f16f7..449c75c25 100644
--- a/datafusion/common/src/scalar.rs
+++ b/datafusion/common/src/scalar.rs
@@ -2341,14 +2341,13 @@ impl ScalarValue {
| ScalarValue::LargeBinary(b) => {
b.as_ref().map(|b| b.capacity()).unwrap_or_default()
}
- // TODO(crepererum): `Field` is NOT fixed size, add
`Field::size` method to arrow (https://github.com/apache/arrow-rs/issues/3147)
ScalarValue::List(vals, field) => {
vals.as_ref()
.map(|vals| Self::size_of_vec(vals) -
std::mem::size_of_val(vals))
.unwrap_or_default()
- + std::mem::size_of_val(field)
+ // `field` is boxed, so it is NOT already included in
`self`
+ + field.size()
}
- // TODO(crepererum): `Field` is NOT fixed size, add
`Field::size` method to arrow (https://github.com/apache/arrow-rs/issues/3147)
ScalarValue::Struct(vals, fields) => {
vals.as_ref()
.map(|vals| {
@@ -2358,11 +2357,14 @@ impl ScalarValue {
+ (std::mem::size_of::<ScalarValue>() *
vals.capacity())
})
.unwrap_or_default()
+ // `fields` is boxed, so it is NOT already included in
`self`
+ + std::mem::size_of_val(fields)
+ (std::mem::size_of::<Field>() * fields.capacity())
+ + fields.iter().map(|field| field.size() -
std::mem::size_of_val(field)).sum::<usize>()
}
- // TODO(crepererum): `DataType` is NOT fixed size, add
`DataType::size` method to arrow
(https://github.com/apache/arrow-rs/issues/3147)
ScalarValue::Dictionary(dt, sv) => {
- std::mem::size_of_val(dt.as_ref()) + sv.size()
+ // `dt` and `sv` are boxed, so they are NOT already
included in `self`
+ dt.size() + sv.size()
}
}
}
diff --git a/datafusion/physical-expr/src/aggregate/approx_percentile_cont.rs
b/datafusion/physical-expr/src/aggregate/approx_percentile_cont.rs
index 2b1a5da85..3cbf9c9cd 100644
--- a/datafusion/physical-expr/src/aggregate/approx_percentile_cont.rs
+++ b/datafusion/physical-expr/src/aggregate/approx_percentile_cont.rs
@@ -416,7 +416,9 @@ impl Accumulator for ApproxPercentileAccumulator {
}
fn size(&self) -> usize {
- // TODO(crepererum): `DataType` is NOT fixed size, add
`DataType::size` method to arrow
(https://github.com/apache/arrow-rs/issues/3147)
std::mem::size_of_val(self) + self.digest.size()
+ - std::mem::size_of_val(&self.digest)
+ + self.return_type.size()
+ - std::mem::size_of_val(&self.return_type)
}
}
diff --git a/datafusion/physical-expr/src/aggregate/array_agg.rs
b/datafusion/physical-expr/src/aggregate/array_agg.rs
index 1eff48ce8..c56a8adb7 100644
--- a/datafusion/physical-expr/src/aggregate/array_agg.rs
+++ b/datafusion/physical-expr/src/aggregate/array_agg.rs
@@ -155,9 +155,10 @@ impl Accumulator for ArrayAggAccumulator {
}
fn size(&self) -> usize {
- // TODO(crepererum): `DataType` is NOT fixed size, add
`DataType::size` method to arrow
(https://github.com/apache/arrow-rs/issues/3147)
std::mem::size_of_val(self) + ScalarValue::size_of_vec(&self.values)
- std::mem::size_of_val(&self.values)
+ + self.datatype.size()
+ - std::mem::size_of_val(&self.datatype)
}
}
diff --git a/datafusion/physical-expr/src/aggregate/array_agg_distinct.rs
b/datafusion/physical-expr/src/aggregate/array_agg_distinct.rs
index bb32a9ffd..bb176e98c 100644
--- a/datafusion/physical-expr/src/aggregate/array_agg_distinct.rs
+++ b/datafusion/physical-expr/src/aggregate/array_agg_distinct.rs
@@ -158,9 +158,10 @@ impl Accumulator for DistinctArrayAggAccumulator {
}
fn size(&self) -> usize {
- // TODO(crepererum): `DataType` is NOT fixed size, add
`DataType::size` method to arrow
(https://github.com/apache/arrow-rs/issues/3147)
std::mem::size_of_val(self) +
ScalarValue::size_of_hashset(&self.values)
- std::mem::size_of_val(&self.values)
+ + self.datatype.size()
+ - std::mem::size_of_val(&self.datatype)
}
}
diff --git a/datafusion/physical-expr/src/aggregate/count_distinct.rs
b/datafusion/physical-expr/src/aggregate/count_distinct.rs
index ead64ebd8..5484c8608 100644
--- a/datafusion/physical-expr/src/aggregate/count_distinct.rs
+++ b/datafusion/physical-expr/src/aggregate/count_distinct.rs
@@ -220,7 +220,6 @@ impl Accumulator for DistinctCountAccumulator {
}
fn size(&self) -> usize {
- // TODO(crepererum): `DataType` is NOT fixed size, add
`DataType::size` method to arrow
(https://github.com/apache/arrow-rs/issues/3147)
std::mem::size_of_val(self)
+ (std::mem::size_of::<DistinctScalarValues>() *
self.values.capacity())
+ self
@@ -231,6 +230,13 @@ impl Accumulator for DistinctCountAccumulator {
})
.sum::<usize>()
+ (std::mem::size_of::<DataType>() *
self.state_data_types.capacity())
+ + self
+ .state_data_types
+ .iter()
+ .map(|dt| dt.size() - std::mem::size_of_val(dt))
+ .sum::<usize>()
+ + self.count_data_type.size()
+ - std::mem::size_of_val(&self.count_data_type)
}
}
diff --git a/datafusion/physical-expr/src/aggregate/median.rs
b/datafusion/physical-expr/src/aggregate/median.rs
index 64d6fa7b4..a04bd5369 100644
--- a/datafusion/physical-expr/src/aggregate/median.rs
+++ b/datafusion/physical-expr/src/aggregate/median.rs
@@ -181,7 +181,6 @@ impl Accumulator for MedianAccumulator {
}
fn size(&self) -> usize {
- // TODO(crepererum): `DataType` is NOT fixed size, add
`DataType::size` method to arrow
(https://github.com/apache/arrow-rs/issues/3147)
std::mem::align_of_val(self)
+ (std::mem::size_of::<ArrayRef>() * self.all_values.capacity())
+ self
@@ -192,6 +191,8 @@ impl Accumulator for MedianAccumulator {
+ array_ref.get_array_memory_size()
})
.sum::<usize>()
+ + self.data_type.size()
+ - std::mem::size_of_val(&self.data_type)
}
}
diff --git a/datafusion/physical-expr/src/aggregate/sum_distinct.rs
b/datafusion/physical-expr/src/aggregate/sum_distinct.rs
index 95823c517..c835419a5 100644
--- a/datafusion/physical-expr/src/aggregate/sum_distinct.rs
+++ b/datafusion/physical-expr/src/aggregate/sum_distinct.rs
@@ -177,9 +177,10 @@ impl Accumulator for DistinctSumAccumulator {
}
fn size(&self) -> usize {
- // TODO(crepererum): `DataType` is NOT fixed size, add
`DataType::size` method to arrow
(https://github.com/apache/arrow-rs/issues/3147)
std::mem::size_of_val(self) +
ScalarValue::size_of_hashset(&self.hash_values)
- std::mem::size_of_val(&self.hash_values)
+ + self.data_type.size()
+ - std::mem::size_of_val(&self.data_type)
}
}
