[python] Fix $files reporting empty min/max value stats - #10111
Conversation
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
This fixes a real end-to-end problem: persisted manifest stats are BinaryRow, and the current $files rendering drops their min/max values. I ran the full files_table_test.py suite (5 passed) and reproduced the new output with a real local table.
[P1] Do not label historical file stats with the current table's column names. The new _row_values(...) path exposes a pre-existing _stats_columns fallback that uses self.base_table.field_names whenever value_stats_cols is absent. But ManifestFileManager decodes the BinaryRow with meta.schema_id's historical fields. In a real metadata.stats-mode=full table I wrote id=1, v='old', dropped v, then added a new v INT. The old file now appears in $files as min_value_stats={"id":1,"v":"old"} and the same max stats: the string value of the deleted column is attributed to a different integer column. The prior behavior displayed {}, so this PR introduces misleading stats. Please align the names and identity with the file's historical stats fields, and cover drop/re-add (or otherwise omit stats for a column whose identity no longer matches). The existing test only covers a stable schema.
This change has user-facing value; I am requesting changes for the schema-evolution correctness issue before production use.
| return values | ||
| try: | ||
| return [row.get_field(i) for i in range(len(row))] | ||
| except Exception: |
There was a problem hiding this comment.
This broad catch turns every stats decoding failure into a valid-looking {} result. For example, if BinaryRow.get_field() raises because the manifest bytes are malformed, a type cannot be decoded, or the resolved fields do not match the stored row, $files silently reports empty statistics - the same success-shaped symptom this PR is fixing. Java’s FilesTable does not suppress these decoding failures.
Please let the actionable exception propagate, or catch only a specifically expected compatibility condition and surface it with the affected file/schema context. A focused test can use a row whose get_field() raises and assert that _row_values does not return [] .
The $files system table rendered min_value_stats and max_value_stats as {}
for every file, even when the manifest carries per-column value stats.
_render_stats_map read the row values via getattr(row, "values", []), but on
the read path the min/max rows are BinaryRow (built in manifest_file_manager),
which exposes get_field/__len__ and has no values attribute -- so getattr
yields [] and the map renders empty. Only GenericRow, the write-path type, has
values, which is why null_value_counts and min_key/max_key render correctly.
Add a _row_values helper that falls back to get_field(i) when values is
absent, and use it for the two value-stats maps. min_key/max_key are
GenericRow, so _render_key is left unchanged. A malformed or schema-evolved
stats row degrades to {} rather than aborting the whole $files listing.
Render min/max/null-count stats through SimpleStatsEvolutions, matching the Java FilesTable, so a dropped or re-added column no longer inherits stats from a column with the same name. Stats decode errors now propagate instead of being rendered as {}.
06fa3df to
1cd4c82
Compare
|
Fixed in the latest push. Value stats now evolve to the current schema by field id via The broad One visible change: a file written with no value stats (Python's default |
3 participants
Purpose
The
$filessystem table renderedmin_value_statsandmax_value_statsas{}for every file: on the read path the stats rows areBinaryRow, which has novaluesattribute.Stats are now read via
get_fieldand evolved to the current schema by field id withSimpleStatsEvolutions, as the JavaFilesTabledoes, so a dropped or re-added column no longer inherits another column's stats. A file written with no value stats lists each column asnull, matching Java. Decode errors propagate instead of rendering{}.Tests
files_table_test: real min/max withmetadata.stats-mode=full; a dropped-and-re-added column showsnullon the old file (test_value_stats_follow_field_ids_after_drop_and_re_add); a decode error is not swallowed.Written with Claude Code; verification is mine.