fix(datafusion): report an unconvertible column statistic as absent - #9973
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
`stats_set_to_df` ran the stored statistic and the column dtype through two `vortex_expect`s per statistic. Both sides come from the file -- the value out of the footer's stats set, the dtype off the column -- so both can legitimately fail on a file we did not write: - `Stat::dtype` returns `None` where the statistic does not apply, which is `Min`/`Max` of a null column and `Sum` of a string, list or struct column; - `Scalar::try_new` rejects a stored value that does not match the column dtype. Either one panicked inside DataFusion planning rather than returning an error or simply forgoing the statistic. Forgoing it is what the end of the same chain already did: `try_to_df().ok()` drops a scalar DataFusion cannot represent. This extends that to the whole conversion and collapses the three near-identical blocks into one helper, which is also what the `TODO(connor)` above them asked for. The statistic's `Precision` is preserved, so an exact statistic stays exact and only the unconvertible ones become `Absent`. Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
stats_set_to_df(vortex-datafusion/src/convert/stats.rs:30-66) ran each statistic through twovortex_expects. Both sides of that conversion come from the file — the stored value out of thefooter's stats set, the dtype off the column — so both can legitimately fail on a file we did not
write:
Stat::dtypereturnsNonewhere the statistic does not apply:vortex-array/src/expr/stats/mod.rs:173,175returnNoneforMax/Minof aDType::Nullcolumn, and
Sum::return_dtypedoes the same for string, binary, list and struct columns. That hitvortex_expect("must have a valid dtype").Scalar::try_newrejects a stored value that does not match the column dtype, hittingvortex_expect("Stat::Minsomehow had an incompatibleDType").Either one panicked inside DataFusion planning instead of returning an error or simply forgoing the
statistic.
Fix
Forgoing it is what the end of the same chain already chose:
try_to_df().ok()drops a scalarDataFusion cannot represent. This extends that to the whole conversion, which also lets the three
near-identical blocks collapse into one helper — and is what the
TODO(connor)above them("There's a lot that can go wrong here, should probably handle this more gracefully") asked for, so
that comment goes too.
The statistic's
Precisionis preserved rather than flattened: an exact statistic staysPrecision::Exact, and only the unconvertible ones becomeAbsent. That distinction is easy to losehere, since
StatsSet::getreturns Vortex'sPrecisionand not anOption.Tests
cargo test --release -p vortex-datafusion: 341 passed, 335 before. Four cases cover theunconvertible shapes —
Min/Maxof a null column,Sumof a string column, and a stored valuethat disagrees with the column dtype — and two more pin that a convertible statistic keeps its exact
or inexact precision.
Restoring the
vortex_expects fails all four unconvertible cases, on both panic sites(
vortex-error/src/lib.rs:666for theOptionand:659for theResult), and leaves theprecision and
distinct_countcases green.AI assistance
Written with agentic AI assistance; my first version returned
Optioninstead ofPrecisionandwould have silently flattened every statistic's precision — the compiler caught it, and the two
precision cases above exist so a reviewer does not have to.