Conversation
ColinLeeo
added a commit
to ColinLeeo/tsfile
that referenced
this pull request
Oct 5, 2026
Revert the statistics changes from dbccee3, including their later formatting, now maintained in apache#989. Keep PR apache#970 focused on table read error propagation and preserve its subsequent error handling changes. Validation: Release build, 136 reader/CLI/statistics/golden/read-failure tests, targeted Spotless, and whitespace checks passed.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unreliable zero-count timelines can still bypass scanning and produce negative null counts, and the fallback branch lacks effective regression coverage.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Preserves aligned time statistics in filtered C++ metadata so CLI table statistics can report numeric null counts.
Changes:
- Reuses offset-based metadata loading to retain shared time indexes.
- Adds scan fallback logic for missing timelines.
- Updates regression tests and golden outputs.
| File | Description |
|---|---|
cpp/src/file/tsfile_io_reader.cc |
Preserves aligned time metadata. |
cpp/tools/commands/cmd_stats.cc |
Expands scan fallback conditions. |
cpp/test/reader/tsfile_reader_test.cc |
Verifies filtered time indexes. |
cpp/test/tools/command_e2e_test.cc |
Adds sparse-field coverage. |
cpp/test/tools/golden/table_stats_csv.txt |
Updates CSV null counts. |
cpp/test/tools/golden/table_stats_ndjson.txt |
Updates NDJSON null counts. |
cpp/test/tools/golden/table_stats_table.txt |
Updates table null counts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+383
to
+385
| if (!it->second.has_timeline_statistic) { | ||
| need_scan = true; | ||
| break; |
Comment on lines
+173
to
+177
| TEST(CliE2E, StatsReportsNumericNullCountForTableFields) { | ||
| // A FIELD whose value statistic is present but whose entity timeline | ||
| // statistic is unavailable must still get a numeric null_count by | ||
| // scanning (TsFile-143): null_count previously stayed empty even though | ||
| // the field's non-null count was known. |
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.
Filtered table metadata discarded the shared time index, so
tsfile-cli statscould report an emptynull_counteven when the footer contained the entity row count. Reuse the existing offset-based metadata reader to retain the aligned time statistics. If the timeline is missing or reports fewer rows than a selected field's non-null count, scan the rows before computingnull_count.Regression coverage checks filtered metadata and sparse table fields. Test-only metadata injection exercises missing, zero-count, and undersized timelines alongside an intact footer. Each case verifies the metadata state and complete CSV output, including a positive null count for the sparse field. Field aggregates still come from footer statistics when available; the fallback supplies the entity row count.
Validation:
TsFile_Testandtsfile_cliin an isolated Release build based ondevelop, with LZ4 enabled and the other optional codecs and ANTLR4 disabled.