alamb commented on code in PR #5937:
URL: https://github.com/apache/arrow-datafusion/pull/5937#discussion_r1163177098
##########
datafusion/physical-expr/src/intervals/cp_solver.rs:
##########
@@ -122,6 +122,18 @@ pub struct ExprIntervalGraph {
root: NodeIndex,
}
+impl ExprIntervalGraph {
+ pub fn size(&self) -> usize {
Review Comment:
I suggest following the same convention as `ScalarValue` and including the
size of `Self` in this calculation:
https://github.com/apache/arrow-datafusion/blob/dafe99733e0f97bfb5ef750f02d02abcb641682d/datafusion/common/src/scalar.rs#L3152-L3154
```suggestion
/// Estimate size of bytes including `Self`.
pub fn size(&self) -> usize {
```
I think that would also require adding size of self to the calculation
##########
datafusion/core/src/physical_plan/joins/symmetric_hash_join.rs:
##########
@@ -1339,6 +1370,11 @@ fn combine_two_batches(
}
impl SymmetricHashJoinStream {
+ fn size(&self) -> usize {
Review Comment:
Should we also include the other fields like the `input_stream`
column_indexes, etc? Maybe that is effectively a constant so it is ok to miss
it in the calculation
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
