xanderbailey commented on PR #26310: URL: https://github.com/apache/datafusion/pull/26310#issuecomment-6189894690
Thanks for the thorough review @andygrove! Going through the main points: **1. Absent min/max wipes out merged stats** Agreed, this is real. The obvious fix doesn't work, though. Giving a null partition `Exact(<typed null>)` min/max (the way `AggregateExec` represents all-null columns) would be worse than `Absent`. `ScalarValue` puts nulls below every value, so `precision_min` would pick the NULL, and `min(d)` answered from statistics would return NULL instead of `2024-01-01`? Is that what we expect to see? Let me look a little more into this. But `Absent` is at least safe and given that before this PR a null partition on a non-string column couldn't be read at all, and the only case that loses stats is string partition columns, which used to return the literal placeholder (which I think is just a bug?) WDYT? **2. `MinMaxStatistics::new_from_files` errors on a null partition** Very nice catch, you were right, fixed in c6f7cd9. **3. `distinct_count = Exact(0)`** I think this one is correct as written but please challenge me. By convention `distinct_count` doesn't count NULLs anywhere in DataFusion: `compute_group_ndv` adds 1 separately when `null_count > 0`, and `max_distinct_count` in the join code documents that nulls don't count as distinct values. So `Exact(0)` doesn't under-report. It also stays exact when `num_rows` is `Absent`. The placeholder directory tells us every row is NULL, so there are zero distinct non-null values however many rows there are. `null_count` does depend on the row count, which is why it follows `num_rows` and becomes `Absent`. I also checked the main consumers: the GROUP BY estimate comes out at 1 group, the join estimate returns no estimate when the selectivity is 0, and filter/limit keep an exact 0. **4. Default-on behaviour change** Agreed it needs calling out. I'll add an entry to the 56.0.0 upgrade guide and update the PR description. On per-table versus session-wide: Hive's `hive.exec.default.partition.name` is itself a session/server setting, and Spark hardcodes `__HIVE_DEFAULT_PARTITION__` with no option at all, so a session setting seems like a reasonable place to start to me? **5. All partition columns become nullable** I don't think we can make nullability conditional on the data. `ListingTable::try_new` builds the schema before any files are listed, and the listing can change between queries, so a schema that depends on what's on disk would be unstable. Spark and Hive also treat partition columns as nullable, so losing the `IS NOT NULL` simplification seems like it might be the right trade-off? Fair point about direct `FileScanConfig` users, though. `pruned_partition_list` is public and can now return null partition values, Is it okay to just document that callers building their own partition fields need them to be nullable? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
