Skip to content

Replace ad-hoc timestamp index guard with dedicated staleness check in isSegmentStale#18310

Merged
gortiz merged 1 commit into
apache:masterfrom
shounakmk219:fix-timestamp-index-is-stale-validation
Apr 28, 2026
Merged

Replace ad-hoc timestamp index guard with dedicated staleness check in isSegmentStale#18310
gortiz merged 1 commit into
apache:masterfrom
shounakmk219:fix-timestamp-index-is-stale-validation

Conversation

@shounakmk219
Copy link
Copy Markdown
Collaborator

Summary

  • The previous fix (Fix false-positive "column deleted" stale check for timestamp-index derived columns #18294) added a narrow guard inside the "column deleted" branch to suppress false positives when a timestamp index is removed. This was incomplete for two reasons raised in the PR review:
    • Removing a timestamp index should mark the segment stale so SegmentPreProcessor can clean up the orphaned $col$GRANULARITY columns — the old guard silently skipped them.
    • Adding a new granularity or configuring a timestamp index for the first time was misreported as "column added" rather than a dedicated timestamp index reason. Existing derived columns in the segment could also trigger spurious "range index changed" verdicts on every reload.
  • Replaces the guard with a dedicated feature-level check (following the StarTree and MultiColumnText pattern): before the per-column loop, compare the set of $col$GRANULARITY columns expected by the current table config against those present in the segment. Any mismatch returns "timestamp index changed".
  • The generic "column added" check now excludes $col$GRANULARITY columns (covered by the dedicated check), and the per-column loop skips them entirely to prevent false positives on "column deleted" and "range index changed".

Test plan

  • Updated 4 existing timestamp index tests to assert "timestamp index changed" with isStale() == true for the add/remove/granularity-added/granularity-removed cases.
  • testTimestampIndexNoChange continues to assert not stale.
  • ./mvnw -pl pinot-core -am -Dtest=BaseTableDataManagerNeedRefreshTest -Dsurefire.failIfNoSpecifiedTests=false test — all 42 tests pass.

🤖 Generated with Claude Code

…n isSegmentStale

The previous fix (apache#18294) added a narrow guard inside the "column deleted" branch
to suppress false positives when a timestamp index is removed. This was incomplete:
- Removing a timestamp index should mark the segment stale (so SegmentPreProcessor
  can clean up the orphaned $col$GRANULARITY columns), but the old guard silently
  skipped them.
- Adding a new granularity or configuring a timestamp index for the first time was
  misreported as "column added" instead of a dedicated timestamp index reason.
- Existing derived columns in the segment could trigger spurious "range index
  changed" verdicts on every reload because applyTimestampIndex injects them into
  the range index config.

This commit replaces the guard with a dedicated feature-level check (following the
StarTree and MultiColumnText pattern). Before the generic per-column loop:
1. Compare the set of $col$GRANULARITY columns expected by the current table config
   against those physically present in the segment. Any mismatch returns
   "timestamp index changed", correctly covering add/remove/granularity changes.
2. The generic "column added" check now excludes $col$GRANULARITY columns (handled
   by step 1).
3. The per-column loop skips all $col$GRANULARITY columns entirely, preventing
   false positives on "column deleted" and "range index changed".

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 23, 2026

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.60%. Comparing base (a239cb3) to head (35d1442).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
.../pinot/core/data/manager/BaseTableDataManager.java 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18310      +/-   ##
============================================
- Coverage     63.61%   63.60%   -0.01%     
  Complexity     1659     1659              
============================================
  Files          3246     3246              
  Lines        197514   197522       +8     
  Branches      30580    30580              
============================================
- Hits         125644   125633      -11     
- Misses        61823    61840      +17     
- Partials      10047    10049       +2     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-11 63.58% <92.30%> (+<0.01%) ⬆️
java-21 63.57% <92.30%> (-0.01%) ⬇️
temurin 63.60% <92.30%> (-0.01%) ⬇️
unittests 63.60% <92.30%> (-0.01%) ⬇️
unittests1 55.57% <92.30%> (-0.01%) ⬇️
unittests2 35.04% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gortiz gortiz merged commit f594601 into apache:master Apr 28, 2026
16 checks passed
@Jackie-Jiang Jackie-Jiang added observability Related to observability (logging, tracing, metrics) index Related to indexing (general) labels Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

index Related to indexing (general) observability Related to observability (logging, tracing, metrics)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants