Uh oh!
There was an error while loading. Please reload this page.
Track column sizes in Statistics; propagate through projections - #19113
Conversation
af67b08 to
874a51dCompare53b5875 to
23ff3e9Compare| let sum_scan_bytes: Option<usize> = self | ||
| .column_statistics | ||
| .iter() | ||
| .map(|cs| cs.byte_size.get_value().copied()) | ||
| .try_fold(0usize, |acc, val| val.map(|v| acc + v)); |
There was a problem hiding this comment.
This needs to handle exactness I think
| s | ||
| }; | ||
| let s = if cs.byte_size != Precision::Absent { | ||
| format!("{} ScanBytes={}", s, cs.byte_size) |
There was a problem hiding this comment.
Need to change to just Bytes
| // 300/1000 = 0.3, so 8000 * 0.3 = 2400 | ||
| assert_eq!(result.total_byte_size, Precision::Inexact(2400)); | ||
| // Column 1: byte_size 800 * (300/500) = 240, Sum = 240 | ||
| assert_eq!(result.total_byte_size, Precision::Inexact(240)); |
There was a problem hiding this comment.
Need to double check these numbers
| // This is the same logic as parquet_column but we start from arrow schema index | ||
| // instead of looking up by name. |
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @adriangb! Just a minor comment nit.
This sent me down a rabbit hole of how we might support stats when there are expressions that change a column's size, but that's way out of scope here.
| /// The number of table rows. | ||
| /// The number of rows estimated to be scanned. | ||
| pub num_rows: Precision<usize>, | ||
| /// Total bytes of the table rows. |
There was a problem hiding this comment.
Since you reworded "table rows" above, we should do something similar here.
There was a problem hiding this comment.
I sent 7a15d26 which actually does a bit more rewording than even you suggested. Could you take a look at the new wording?
adriangb
commented
Dec 10, 2025
Yes agreed, I think that's a beneficial but orthogonal change. There's already some stuff present in |
23ff3e9 to
7a15d26Comparembutrovich
left a comment
There was a problem hiding this comment.
LGTM, thanks @adriangb! Looks like it just needs a cargo fmt and for CI to go green.
Closes#19098, follow up to #19094. Related to #14936