You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "bert-beyondloops (via GitHub)" <gi...@apache.org> on 2026/07/31 12:14:27 UTC
[PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
bert-beyondloops opened a new pull request, #24027:
URL: https://github.com/apache/datafusion/pull/24027
## Which issue does this PR close?
- Closes #24026
## Rationale for this change
`Statistics::calculate_total_byte_size `is meant to derive `total_byte_size` from `num_rows` and the schema's fixed-width columns. When all columns have a primitive width but `num_rows` is `Precision::Absent`, the old code computed
`self.num_rows.multiply(&Precision::Exact(size))`, and `Precision::multiply` returns `Precision::Absent` whenever either operand is `Absent`. This silently overwrote any previously known `total_byte_size` (exact or inexact) with
`Absent`, even though the non-primitive-width branch already handled this situation correctly by downgrading the existing value to inexact instead of discarding it.
## What changes are included in this PR?
- In `Statistics::calculate_total_byte_size`, when the schema is all fixed-width but num_rows is` Precision::Absent,` keep the existing `total_byte_size` and downgrade it to inexact via `to_inexact(),` instead of overwriting it with
`Absent`.
- Updated the doc comment on `calculate_total_byte_size` to describe this behavior.
- Added `test_calculate_total_byte_size` covering: an all-primitive schema with known row count (exact size), an all-primitive schema with unknown row count (preserved but downgraded to inexact), and a non-primitive schema
(always downgraded to inexact regardless of row count).
## Are these changes tested?
Yes — added `stats::tests::test_calculate_total_byte_size, exercising all three branches of the updated match.
## Are there any user-facing changes?
No public API changes. Statistics propagation is more accurate (previously known total_byte_size estimates are no longer dropped to Absent when num_rows is unknown), which may result in slightly better cost-based planning
decisions in some cases.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "bert-beyondloops (via GitHub)" <gi...@apache.org>.
bert-beyondloops commented on PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#issuecomment-5218012644
🙏 @AdamGS @asolimando @pepijnve
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#issuecomment-5192304417
That's the part I understand, but say we have a schema with a single string column and the table provider (or any other source) reports exact size and and exact row count, my understanding is that the current behavior (even before this PR) will always set it to `Inexact`.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS merged PR #24027:
URL: https://github.com/apache/datafusion/pull/24027
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#issuecomment-5191584458
> This looks good to me overall, just one small test comment.
>
> I'm not sure I fully understand the existing behavior that always downgrades non-primitive schemas to inexact, but I also recall that it doesn't make a huge difference.
Hey Adam, my understanding is that non-primitive types (composite types, arrays, list, maps) contribute a varying amount of bytes, depending on the size of the collection, which cannot be really known, so it's inexact by nature.
Primitive types, on the contrary, are tied to a fixed byte-size which can be computed exactly (so given a schema composed only by primitive types, you can tell how many bytes a single row takes, and total byte size is row byte size times number of rows).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "bert-beyondloops (via GitHub)" <gi...@apache.org>.
bert-beyondloops commented on code in PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#discussion_r3726808167
##########
datafusion/common/src/stats.rs:
##########
@@ -3345,4 +3347,33 @@ mod tests {
precision_add_for_sum_in_place(&mut lhs, &Precision::Absent);
assert_eq!(lhs, Precision::Absent);
}
+
+ #[test]
+ fn test_calculate_total_byte_size() {
+ let primitive_schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
+ let non_primitive_schema =
+ Schema::new(vec![Field::new("a", DataType::Utf8, false)]);
+
+ // All-primitive schema with a known row count computes an exact size.
+ let mut stats = Statistics::new_unknown(&primitive_schema);
+ stats.num_rows = Precision::Exact(10);
+ stats.calculate_total_byte_size(&primitive_schema);
+ assert_eq!(stats.total_byte_size, Precision::Exact(40));
+
+ // All-primitive schema with an unknown row count keeps a previously
+ // known `total_byte_size`, downgraded to inexact, instead of
+ // discarding it to `Absent`.
+ let mut stats = Statistics::new_unknown(&primitive_schema);
+ stats.total_byte_size = Precision::Inexact(1234);
Review Comment:
Yes, that makes sense. I changed it.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#discussion_r3719994742
##########
datafusion/common/src/stats.rs:
##########
@@ -3345,4 +3347,33 @@ mod tests {
precision_add_for_sum_in_place(&mut lhs, &Precision::Absent);
assert_eq!(lhs, Precision::Absent);
}
+
+ #[test]
+ fn test_calculate_total_byte_size() {
+ let primitive_schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
+ let non_primitive_schema =
+ Schema::new(vec![Field::new("a", DataType::Utf8, false)]);
+
+ // All-primitive schema with a known row count computes an exact size.
+ let mut stats = Statistics::new_unknown(&primitive_schema);
+ stats.num_rows = Precision::Exact(10);
+ stats.calculate_total_byte_size(&primitive_schema);
+ assert_eq!(stats.total_byte_size, Precision::Exact(40));
+
+ // All-primitive schema with an unknown row count keeps a previously
+ // known `total_byte_size`, downgraded to inexact, instead of
+ // discarding it to `Absent`.
+ let mut stats = Statistics::new_unknown(&primitive_schema);
+ stats.total_byte_size = Precision::Inexact(1234);
Review Comment:
Should this start as `Exact` so it actually downgrades?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#issuecomment-5194142976
> That's the part I understand, but say we have a schema with a single string column and the table provider (or any other source) reports exact size and and exact row count, my understanding is that the current behavior (even before this PR) will always set it to `Inexact`.
OK, I see what you mean.
The only caller of this function today is [projection.rs#L724](https://github.com/apache/datafusion/blob/152d8c47eb6b7ddbaa40ec26f24d714c24572d40/datafusion/physical-expr/src/projection.rs#L724): if we used `byte_size` from `ColumnStatistics` instead of `DataType::primitive_width()`, I think we could keep exact (provided that the column-level byte sizes are exact too), even for non-primitive types (length varying types).
This of course depends on the projection expressions, if they are pure input column references (dropping some columns, for instance), and that columns have exact byte size values, it would be doable.
When you say it doesn't matter much in practice I guess it's because Parquet sets `Inexact` for non-primitive data types.
If you agree on my reading and it feels useful, I can file a follow-up issue to track this.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#issuecomment-5142998231
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24027?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.85%. Comparing base ([`dbcb5c0`](https://app.codecov.io/gh/apache/datafusion/commit/dbcb5c0f729e9ef6b0ab4c79253fe3b657929f48?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5496845`](https://app.codecov.io/gh/apache/datafusion/commit/549684576847618afb81243ce039bd5cc9d2ba42?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24027 +/- ##
==========================================
- Coverage 80.85% 80.85% -0.01%
==========================================
Files 1101 1101
Lines 374933 374951 +18
Branches 374933 374951 +18
==========================================
+ Hits 303166 303177 +11
- Misses 53671 53675 +4
- Partials 18096 18099 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24027?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#discussion_r3702345743
##########
datafusion/common/src/stats.rs:
##########
@@ -441,11 +443,11 @@ impl Statistics {
}
}
}
- match row_size {
- None => {
+ match (row_size, &self.num_rows) {
+ (None, _) | (Some(_), Precision::Absent) => {
Review Comment:
This change makes sense to me since it avoids downgrading `Exact` `total_byte_size` to absent when `num_rows` is `Absent`.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve total_byte_size in calculate_total_byte_size when num_r… [datafusion]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #24027:
URL: https://github.com/apache/datafusion/pull/24027#discussion_r3702614646
##########
datafusion/common/src/stats.rs:
##########
@@ -441,11 +443,11 @@ impl Statistics {
}
}
}
- match row_size {
- None => {
+ match (row_size, &self.num_rows) {
+ (None, _) | (Some(_), Precision::Absent) => {
Review Comment:
Intuitively this makes sense to me too, hopefully we will soon be able to have a more data-driven approach thanks to https://github.com/apache/datafusion/pull/23975.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org