Skip to content

fix(spec): implement Display for BinaryTableStats - #632

Open
u70b3 wants to merge 1 commit into
apache:mainfrom
u70b3:fix/binary-table-stats-display
Open

fix(spec): implement Display for BinaryTableStats#632
u70b3 wants to merge 1 commit into
apache:mainfrom
u70b3:fix/binary-table-stats-display

Conversation

@u70b3

@u70b3 u70b3 commented Jul 30, 2026

Copy link
Copy Markdown

Purpose

Linked issue: close #631

The Display impl for BinaryTableStats in spec/stats.rs is the repository's only todo!() and would panic if any code path ever formats stats through Display (logging, debugging, error messages). This implements it.

Brief change log

Implement Display for BinaryTableStats as a labeled summary, following the convention of DataFileMeta's Display:

BinaryTableStats{minValues=[0, 0, 0, 2, 1], maxValues=[0, 0, 0, 2, 9], nullCounts=[Some(0), None, Some(3)]}
  • min_values/max_values are serialized BinaryRows that cannot be decoded without the column types, so they print as raw bytes — the same convention DataFileMeta uses for its minKey/keyStats fields.
  • null_counts items are Option<i64> (None = unknown), which {:?} renders naturally.

Assumptions and notes for reviewers

  • No current caller formats stats via Display: ManifestFileMeta and DataFileMeta print BinaryTableStats fields via {:?} (Debug), so this change is preventive — it removes a panic landmine rather than fixing a live crash. The alternative (deleting the unused impl) was considered and rejected: BinaryTableStats is a public spec-layer type and Display is a basic debugging capability, so implementing it is the additive, non-breaking choice.
  • The output format is a proposal: raw bytes mirror the DataFileMeta convention; a byte-length summary (e.g. minValues=12 bytes) would also work. Happy to switch if reviewers prefer.

Tests

  • spec::stats::tests::display_formats_labeled_fields — exact output for known bytes, including Some/None null-count mixes
  • spec::stats::tests::display_empty_stats_does_not_panicBinaryTableStats::empty() boundary case

cargo test -p paimon --lib spec:: passes (450 tests); cargo fmt and clippy are clean.

API and Format

No API or storage format change; this fills in a previously unimplemented trait method.

Documentation

No documentation update needed.

🤖 Generated with Claude Code

Replace the repository's only todo!() with a labeled summary in the
style of DataFileMeta's Display. min/max values are serialized
BinaryRows that cannot be decoded without the column types, so they
print as raw bytes, matching how DataFileMeta prints minKey/keyStats.

@QuakeWang QuakeWang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the only todo!() in the repo: implement Display for BinaryTableStats

2 participants