[core] Add tests and implement withFilter for StatisticTable#7893
Conversation
Adds StatisticTableTest and removes the two // TODO markers in StatisticTable.
d69a030 to
afa435f
Compare
CI FailuresThese appear to be unrelated flaky tests, not caused by this PR's changes:
I also want to separate new pr to address the CDC ITCase flakiness. |
afa435f to
86742b2
Compare
86742b2 to
cba1d34
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
Review: StatisticTable withFilter implementation
Overall this is a clean, well-structured change that resolves the TODO markers and adds solid test coverage. A few observations:
Correctness
Filter-before-projection ordering is correct. The postFilter is applied to the full-schema row produced by toRow(), and projection via ProjectedRow happens afterward. This ensures predicate column indices align with TABLE_TYPE rather than the projected readType.
Scan-level withFilter is intentionally a no-op -- since StatisticTable always produces at most one split, there is nothing to prune at the scan layer. The actual row-level filtering is delegated to StatisticRead.withFilter, which is the right approach for this table.
Minor suggestion
The Iterators.filter(rows, postFilter::test) call: since Statistics yields at most one row, the practical effect of the filter is a simple keep-or-discard decision. The current implementation is fine and consistent with how other system tables handle filtering.
Tests
Good coverage: empty statistics, basic read, predicate hit/miss on snapshot_id, and range predicate on merged_record_count. One optional enhancement for the future: a test combining withFilter and withProjection together to validate the filter-then-project ordering invariant.
Summary
The change is correct and well-tested. LGTM.
|
+1 |
Purpose
Remove the two
// TODOmarkers onStatisticTableTests
Adds
StatisticTableTest