feat!: expose read-side expected stats schemas - #3308
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3308 +/- ##
==========================================
+ Coverage 90.26% 90.32% +0.06%
==========================================
Files 251 250 -1
Lines 88345 89100 +755
Branches 88345 89100 +755
==========================================
+ Hits 79742 80477 +735
+ Misses 5730 5687 -43
- Partials 2873 2936 +63 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
breaking-change
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change is a clean refactor plus an additive public API. Splitting the physical-only builder (build_expected_physical_stats_schema, returning SchemaRef) from the new logical+physical build_expected_stats_schemas, and updating the scan, checkpoint, and transaction call sites, is a faithful mechanical extraction with no semantic drift. The physical schema still uses physical names and strips all field metadata (no columnMapping.physicalName, no parquet.field.id), which is correct for stats read from JSON commits and checkpoint Parquet. The new Snapshot::expected_stats_schemas produces output aligned with the scan path, and the added parametrized test pins that contract across all three column-mapping modes.
No blocking issues.
Non-blocking notes
Nit1: kernel/src/table_configuration.rs:307 -- build_expected_stats_schemas resolves extra_indexed_columns against logical_schema_without_partition_columns(), while the scan path's resolve_physical_columns (scan/state_info.rs) resolves against the full logical_schema(). The two produce identical physical schemas today because both ultimately build from partition-excluded schemas, so this is not a current defect, only a maintenance risk from two hand-rolled resolvers that could drift on the "aligned with scan output" guarantee this API exists to provide. Raised by: architecture-reviewer, maintainer-claude-reviewer, delta-protocol-reviewer (disprove gate: NITPICK). Suggested fix: route both entry points through one shared best-effort resolver, or add a short comment at both sites noting the intentional schema choice.
Nit2: kernel/src/scan/tests.rs:2277 -- the consistency test covers a flat two-column schema and asserts the physical minValues field count, but does not assert the logical side's field count and does not exercise partition-column exclusion through the new API's logical branch or a nested/dataSkippingStatsColumns-configured schema. Raised by: test-coverage-reviewer, delta-protocol-reviewer, maintainer-codex-reviewer. Suggested fix: add assert_eq!(logical_min_values.num_fields(), 2); and a partitioned-table case asserting the partition column is absent from both logical and physical.
Nit3: kernel/src/table_configuration.rs:380 -- #[allow(unused)] on build_expected_physical_stats_schema looks stale now that checkpoint, scan, data-skipping, and transaction all call it unconditionally. Raised by: maintainer-claude-reviewer. Suggested fix: drop the attribute if the crate still builds clean under all feature combinations, so it does not mask future dead-code warnings.
Nit4: kernel/src/snapshot/mod.rs:362 -- the # Errors section ("Returns an error if kernel cannot construct a valid stats schema") only restates the return type, and the doc does not mention that both returned schemas are metadata-stripped even though the logical field is described as connector-facing. Raised by: maintainer-claude-reviewer. Suggested fix: name the concrete error condition (or drop the section) and add a one-line note that both schemas have field metadata stripped.
Nit5: kernel/src/table_configuration.rs:357 -- the ASCII schema diagram on build_expected_physical_stats_schema lists numRecords, nullCount, minValues, and maxValues but omits tightBounds, which the builder always emits. The new Snapshot::expected_stats_schemas rustdoc names tightBounds correctly, so the two docs disagree. Raised by: docs-reviewer. Suggested fix: add tightBounds: boolean, to the diagram.
Nit6: kernel/src/snapshot/mod.rs:368 -- expected_stats_schemas(&[ColumnName]) models only the all_struct_with_extra_indexed stats policy; a connector scanning with struct_columns(...) gets a schema that can disagree with its own scan output, which is the mismatch this API is meant to prevent. Raised by: architecture-reviewer. Suggested fix: consider accepting the scan's StatsOptions (or the StructStats policy) so one method covers all modes, or scope the method name to the single policy it serves.
Summary
The refactor preserves protocol semantics and the new public API is additive and test-covered for its intended mode. There are no blocking issues. The notes above are maintenance and coverage improvements worth considering before the public signature ships.
Automated review - workflow run
| .iter() | ||
| .filter_map(|logical_column| { | ||
| get_any_level_column_physical_name( | ||
| &self.logical_schema_without_partition_columns(), |
There was a problem hiding this comment.
Nit1 build_expected_stats_schemas resolves extra_indexed_columns against logical_schema_without_partition_columns(), while the scan path's resolve_physical_columns resolves against the full logical_schema(). Physical schemas agree today (both build from partition-excluded schemas), so this is not a current defect, only a drift risk between two hand-rolled resolvers on the exact 'aligned with scan output' guarantee this API provides. Raised by: architecture-reviewer, maintainer-claude-reviewer, delta-protocol-reviewer. Suggested fix: route both entry points through one shared resolver, or add a comment at both sites noting the intended schema choice.
| #[case::no_column_mapping(None)] | ||
| #[case::name_column_mapping(Some("name"))] | ||
| #[case::id_column_mapping(Some("id"))] | ||
| fn snapshot_expected_stats_schemas_match_scan_output(#[case] column_mapping_mode: Option<&str>) { |
There was a problem hiding this comment.
Nit2 The consistency test covers only a flat two-column schema, asserts the physical minValues field count but not the logical side, and does not exercise partition-column exclusion through the new API's logical branch or a nested/dataSkippingStatsColumns schema. Raised by: test-coverage-reviewer, delta-protocol-reviewer, maintainer-codex-reviewer. Suggested fix: add assert_eq!(logical_min_values.num_fields(), 2); and a partitioned-table case asserting the partition column is absent from both logical and physical.
| @@ -323,11 +379,11 @@ impl TableConfiguration { | |||
| /// <https://github.com/delta-io/delta/blob/master/PROTOCOL.md#per-file-statistics> | |||
| #[allow(unused)] | |||
There was a problem hiding this comment.
Nit3 #[allow(unused)] on build_expected_physical_stats_schema looks stale now that checkpoint, scan, data-skipping, and transaction all call it unconditionally. Raised by: maintainer-claude-reviewer. Suggested fix: drop the attribute if the crate still builds clean under all feature combinations, so it does not mask future dead-code warnings.
| /// Pass the same extra columns to [`StatsOptions::all_struct_with_extra_indexed`] when building | ||
| /// a scan that returns structured statistics. | ||
| /// | ||
| /// # Errors |
There was a problem hiding this comment.
Nit4 The # Errors section only restates the return type, and the doc does not mention that both returned schemas are metadata-stripped even though the logical field is described as connector-facing. Raised by: maintainer-claude-reviewer. Suggested fix: name the concrete error condition (or drop the section) and note that both schemas have field metadata stripped.
| /// an `ExpectedStatsSchemas`. | ||
| /// data skipping and other optimizations. | ||
| /// | ||
| /// The schema is structured as: |
There was a problem hiding this comment.
Nit5 The ASCII schema diagram on build_expected_physical_stats_schema lists numRecords, nullCount, minValues, maxValues but omits tightBounds, which the builder always emits; the new Snapshot::expected_stats_schemas rustdoc names it, so the two docs disagree. Raised by: docs-reviewer. Suggested fix: add tightBounds: boolean, to the diagram.
| /// | ||
| /// [`StatsOptions::all_struct_with_extra_indexed`]: | ||
| /// crate::scan::StatsOptions::all_struct_with_extra_indexed | ||
| pub fn expected_stats_schemas( |
There was a problem hiding this comment.
Nit6 expected_stats_schemas(&[ColumnName]) models only the all_struct_with_extra_indexed policy; a connector scanning with struct_columns(...) gets a schema that can disagree with its own scan output, the mismatch this API aims to prevent. Raised by: architecture-reviewer. Suggested fix: consider accepting the scan's StatsOptions (or the StructStats policy) so one method covers all modes, or scope the method name to the single policy it serves.
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 5 · ☑️ 8 · 🚧 2 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
sanujbasu
changed the title
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. This is a clean, additive change: one logical column selection is mapped to physical names and both schemas are built from structurally identical inputs, so logical/physical alignment holds by construction. The rename to build_expected_physical_stats_schema is applied consistently across the scan, checkpoint, transaction, and data-skipping call sites, and the new tests pin the alignment and scan-consistency contracts well.
Non-blocking notes
Nit1 - kernel/src/table_configuration.rs (build_expected_stats_schemas, physical build around line 366)
The physical schema advertised by expected_stats_schemas is produced by a selection pass (stats_column_names on the logical schema, mapped to physical, rebuilt with num_indexed_cols=None) that is separate from the pass the scan uses to build physical_stats_output_schema (build_expected_physical_stats_schema applying num_indexed_cols directly). The two agree today and the tests assert scan.physical_stats_output_schema == expected.physical, so this is not a current defect. The maintenance cost is that the "advertised schema equals scan-emitted schema" invariant lives in two selection pipelines and is guarded only by the pinned test cases, so a future change to either selector could desynchronize them silently.
Raised by: architecture-reviewer, delta-protocol-reviewer, maintainer-claude-reviewer
Suggested fix: derive the physical field by delegating to build_expected_physical_stats_schema(Some(&physical_columns), Some(&physical_columns)) so both paths share one physical-materialization pipeline, or add a short comment at the physical build pointing at the invariant and the guarding test.
Nit2 - kernel/src/table_configuration.rs (extra column resolution around line 320)
Each extra_indexed_columns entry is resolved to a physical name once for warn-and-skip validation (the result is discarded) and again when the selected logical_columns are mapped to physical_columns. Both calls use the same logical schema and mapping mode, so they cannot disagree today; this is only minor duplicated work.
Raised by: maintainer-claude-reviewer, maintainer-codex-reviewer
Suggested fix: carry the resolved physical name from the first pass instead of recomputing it.
Summary
The change exposes read-side logical and physical stats schemas correctly: per-file stats shape, column-mapping name substitution, id-mode field-id stripping, partition-column exclusion, extra-indexed inclusion beyond the budget, and None-when-no-data-columns all match the protocol and stay consistent with the existing scan and write paths. No correctness, protocol, or safety defects were found in the diff. The only items are two non-blocking maintainability notes about the dual physical-schema selection path and a duplicated resolution step.
Automated review - workflow run
| }) | ||
| .collect::<DeltaResult<Vec<_>>>()?; | ||
|
|
||
| let logical = build_stats_schema_for_columns(&logical_schema, &logical_columns)?; |
There was a problem hiding this comment.
Nit1 The physical schema advertised here is built by a selection pass separate from the one the scan uses for physical_stats_output_schema (build_expected_physical_stats_schema applying num_indexed_cols directly). They agree today and tests assert scan.physical_stats_output_schema == expected.physical, so this is not a current defect, but the equality invariant now lives in two selection pipelines guarded only by the pinned tests and could desynchronize on a future change to either selector. Raised by: architecture-reviewer, delta-protocol-reviewer, maintainer-claude-reviewer. Suggested fix: derive the physical field via build_expected_physical_stats_schema(Some(&physical_columns), Some(&physical_columns)) so both paths share one pipeline, or add a comment pinning the invariant and its guarding test.
| ) -> DeltaResult<Option<ExpectedStatsSchemas>> { | ||
| let logical_schema = self.logical_schema_without_partition_columns(); | ||
| let column_mapping_mode = self.column_mapping_mode(); | ||
| let required_logical_columns: Vec<_> = extra_indexed_columns |
There was a problem hiding this comment.
Nit2 Each extra_indexed_columns entry is resolved to a physical name once for warn-and-skip validation (result discarded) and again when logical_columns are mapped to physical_columns. Both calls use the same logical schema and mapping mode, so they cannot disagree today; this is only minor duplicated work. Raised by: maintainer-claude-reviewer, maintainer-codex-reviewer. Suggested fix: carry the resolved physical name from the first pass instead of recomputing it.
dengsh12
left a comment
dengsh12
left a comment
There was a problem hiding this comment.
LGTM with minor comments.
| self.table_configuration.logical_schema() | ||
| } | ||
|
|
||
| /// Returns aligned schemas for scans using all indexed structured statistics. |
There was a problem hiding this comment.
NIT: This seems not fully accurate? We are returning stats schemas for all indexed columns + caller specified columns
| pub struct ExpectedStatsSchemas { | ||
| /// Schema using logical table column names. | ||
| pub logical: SchemaRef, | ||
| /// Schema using physical column names as encoded in Delta statistics. |
There was a problem hiding this comment.
NIT: From the logic we build it, seems physical col names come from column mapping not the on-disk delta stats?
| /// Schema using physical column names as encoded in Delta statistics. | |
| /// Schema using physical column names. |
| let physical_columns = logical_columns | ||
| .iter() | ||
| .map(|logical_column| { | ||
| get_any_level_column_physical_name( | ||
| &logical_schema, | ||
| logical_column, | ||
| column_mapping_mode, | ||
| ) | ||
| }) | ||
| .collect::<DeltaResult<Vec<_>>>()?; |
There was a problem hiding this comment.
On the above code we emit warning but here we emit error, wonder the reason for the asymmetry? Feel like both can just be warning, as the schema is already validated during construction, the error won't really happen. Then we can remove error from the pub api expected_stats_schemas
pub fn expected_stats_schemas(
&self,
extra_indexed_columns: &[ColumnName],
) -> Option<ExpectedStatsSchemas>
Defer the choice to you -- not blocker
| data_skipping_stats_columns: Some(selected_columns), | ||
| data_skipping_num_indexed_cols: None, | ||
| }; | ||
| let schema = Arc::new(expected_stats_schema(data_schema, &config, None, None)?); |
There was a problem hiding this comment.
NIT: inline comments for None
| assert!(info.field("name").is_some()); | ||
| assert!(info.field("age").is_none()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Wonder if we want to test:
- extra_indexed_columns intersect with the table's configured indexed columns
- extra_indexed_columns includes partition columns
- extra_indexed_columns contains a struct
- IIUC, the expected behavior is: all leaf columns inside the struct will be include
- extra_indexed_columns contains map/array
- IIUC the expected behavior is only nullCount, not min/max values
| /// `extra_indexed_columns` are logical column paths that may have statistics even when they | ||
| /// fall outside the table's configured indexed-column set. Pass the same columns to | ||
| /// [`StatsOptions::all_struct_with_extra_indexed`] when building the scan. Partition columns | ||
| /// and unresolvable paths are omitted. |
There was a problem hiding this comment.
Codex suggested providing a scan::stats_output_schemas instead of this. Seems making sense? It avoid the case that someone pass X to expected_stats_schemas then pass Y to ScanBuilder. But I'm unsure if someone wants to have the stats schema before scan -- defer the choice to you
2 participants
What changes are proposed in this pull request?
Expose the resolved read-side file-statistics schema through
Snapshot. The new API returnsaligned logical and physical schemas, applies column mapping, excludes partition columns, and lets
callers include extra indexed columns beyond the configured statistics budget. It returns
Nonewhen the matching scan would not emit structured statistics.
Internal scan, checkpoint, and transaction paths continue to use the physical schema builder. The
logical and physical schemas now share one column-selection pass, keeping their field order and
shape aligned.
This PR affects the following public APIs
Snapshot::expected_stats_schemas, returning aligned schemas when the scan emits structuredstatistics.
ExpectedStatsSchemasand marks the type#[non_exhaustive].TableConfiguration::build_expected_stats_schemasinternal API tobuild_expected_physical_stats_schemaand returns its physicalSchemaRefdirectly.How was this change tested?
Added scan-schema consistency tests covering excluded columns, explicit nested stats columns,
partition columns, unresolved extras, empty stats selection, and no/name/id column mapping. The
tests also verify logical-to-physical field alignment and metadata removal.