Skip to content

Skip unknown index types in SingleFileIndexDirectory#17992

Merged
xiangfu0 merged 1 commit intoapache:masterfrom
xiangfu0:skip-unknown-index-type
Mar 26, 2026
Merged

Skip unknown index types in SingleFileIndexDirectory#17992
xiangfu0 merged 1 commit intoapache:masterfrom
xiangfu0:skip-unknown-index-type

Conversation

@xiangfu0
Copy link
Copy Markdown
Contributor

Summary

  • Gracefully handle unknown index types when loading SingleFileIndexDirectory by catching IllegalArgumentException from IndexKey.fromIndexName()
  • Logs a warning and skips the unknown entry instead of crashing, improving forward compatibility when segments contain index types not yet recognized by the current version

Test plan

  • Build pinot-segment-local module: ./mvnw -pl pinot-segment-local -am verify
  • Verify existing SingleFileIndexDirectoryTest passes
  • Test with a segment containing an unknown index type entry in the index map to confirm it logs a warning and loads successfully

🤖 Generated with Claude Code

Gracefully handle unknown index types in the index map by logging a
warning and skipping the entry instead of throwing an exception. This
improves forward compatibility when segments contain index types not
yet recognized by the current version.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@xiangfu0 xiangfu0 added the index Related to indexing (general) label Mar 26, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Improves forward compatibility of segment loading by preventing SingleFileIndexDirectory from failing when encountering index map entries for index types not recognized by the running Pinot version.

Changes:

  • Catch IllegalArgumentException from IndexKey.fromIndexName() during index map load.
  • Log a warning and skip unknown index entries instead of throwing and aborting load.

Comment on lines +239 to +242
} catch (IllegalArgumentException e) {
LOGGER.warn("Skipping unknown index type: {} for column: {} in segment: {}", parsedKeys[1], parsedKeys[0],
_segmentDirectory);
continue;
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

This warning will be emitted once per map entry. Since each index typically has at least START_OFFSET and SIZE entries, an unknown index type will likely log the same warning multiple times (log spam). Consider deduplicating (e.g., keep a Set of (column,indexType) already-warned) or lowering to DEBUG after the first occurrence.

Copilot uses AI. Check for mistakes.
Comment on lines +236 to +243
IndexKey indexKey;
try {
indexKey = IndexKey.fromIndexName(parsedKeys[0], parsedKeys[1]);
} catch (IllegalArgumentException e) {
LOGGER.warn("Skipping unknown index type: {} for column: {} in segment: {}", parsedKeys[1], parsedKeys[0],
_segmentDirectory);
continue;
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

This change alters load behavior to skip unknown index types instead of failing. Please add a unit test that writes an index map entry with an unrecognized index id and asserts that SingleFileIndexDirectory still loads (and that known indices remain accessible).

Copilot generated this review using guidance from repository custom instructions.
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.26%. Comparing base (57589a1) to head (f25fc33).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
.../local/segment/store/SingleFileIndexDirectory.java 40.00% 3 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             master   #17992       +/-   ##
=============================================
+ Coverage     34.20%   63.26%   +29.06%     
- Complexity      795     1541      +746     
=============================================
  Files          3198     3200        +2     
  Lines        193998   194042       +44     
  Branches      29865    29870        +5     
=============================================
+ Hits          66348   122767    +56419     
+ Misses       122065    61632    -60433     
- Partials       5585     9643     +4058     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-11 63.24% <40.00%> (+29.04%) ⬆️
java-21 63.23% <40.00%> (-36.77%) ⬇️
temurin 63.26% <40.00%> (+29.06%) ⬆️
unittests 63.26% <40.00%> (+29.06%) ⬆️
unittests1 55.48% <40.00%> (?)
unittests2 34.22% <40.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.

@xiangfu0 xiangfu0 merged commit e53aa21 into apache:master Mar 26, 2026
20 checks passed
@xiangfu0 xiangfu0 deleted the skip-unknown-index-type branch March 26, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

index Related to indexing (general)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants