Add unit tests for IFSTIndexHandler index lifecycle

[Akanksha-kedia]

Summary

IFSTIndexHandler (case-Insensitive FST) had no unit test coverage. It shares the same index lifecycle pattern as FSTIndexHandler but operates on a distinct index type (StandardIndexes.ifst()) with its own file extension. Adds IFSTIndexHandlerTest with 5 tests:

Test	Scenario
testNeedUpdateReturnsTrueWhenColumnRemovedFromConfig	Column has index but dropped from config → needUpdateIndices returns true
testUpdateIndicesRemovesIndexWhenColumnDroppedFromConfig	Dropped column → writer.removeIndex called
testNeedUpdateReturnsTrueWhenNewColumnAdded	Column in config, no index, metadata present → true
testNeedUpdateReturnsFalseWhenColumnMetadataAbsent	Column in config but getColumnMetadataFor returns null → false
testNeedUpdateReturnsFalseWhenIndexUpToDate	Index present and in config → false

Test plan

• IFSTIndexHandlerTest — 5 unit tests, all green
• ./mvnw checkstyle:check -pl pinot-segment-local — 0 violations

[Akanksha-kedia]

cc @xiangfu0 @J-HowHuang @Jackie-Jiang

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.08%. Comparing base (bcf3f66) to head (06b5a73).
⚠️ Report is 54 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18963      +/-   ##
============================================
+ Coverage     64.86%   65.08%   +0.22%     
- Complexity     1347     1403      +56     
============================================
  Files          3392     3407      +15     
  Lines        211663   213087    +1424     
  Branches      33306    33606     +300     
============================================
+ Hits         137290   138684    +1394     
+ Misses        63296    63231      -65     
- Partials      11077    11172      +95     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (?)
java-21	65.08% <ø> (+0.22%)	⬆️
temurin	65.08% <ø> (+0.22%)	⬆️
unittests	65.08% <ø> (+0.22%)	⬆️
unittests1	56.85% <ø> (-0.14%)	⬇️
unittests2	37.47% <ø> (+0.24%)	⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

[Jackie-Jiang]

The newly added test is failing. Please take a look

[Akanksha-kedia]

@xiangfu0 please take a look when you get a chance

[Akanksha-kedia]

@xiangfu0 @shounakmk219 would appreciate a review when you get a chance. This PR adds unit tests for Add unit tests for IFSTIndexHandler index lifecycle.

[gortiz]

Thanks for closing this coverage gap — IFSTIndexHandler sharing FST's lifecycle logic but having zero tests was an easy thing to miss, and these 5 cases (drop/add/no-op/metadata-absent) map cleanly onto the actual branches in needUpdateIndices/updateIndices. Nice work.

A couple of style nits before merging, mostly so this matches conventions we'd like to see more broadly in the codebase:

1. Doc comment style — could you convert the class-level Javadoc to the newer JEP 467 Markdown style (contiguous /// lines) instead of the classic /** ... */ block with HTML <p>/<ul>/<li> tags? E.g.:

   /// Unit tests for [IFSTIndexHandler].
   ///
   /// IFST (case-insensitive FST) indexes share the same index lifecycle as FST indexes but
   /// use a separate index type ([StandardIndexes#ifst()]) and file extension.
   ///
   /// Covers:
   /// - Index removal when a column is dropped from the IFST index config
   /// - New index creation when a column is added to the config
   /// - No update required when the index is already present and matches config
   /// - No update when column metadata is absent (column not in segment)

   This reads more cleanly as plain Markdown and is the direction we'd like new test/doc comments to move in.

2. // ---- section divider ---- comments — could you drop these? They're not a pattern used elsewhere in this test package (or the module generally), and the test method names are already descriptive enough to self-group without banner comments. Blank lines between tests are sufficient.

3. Minor: the class doc's bullet list could call out the "index removed via updateIndices" case explicitly (currently implied under the first bullet) since it's its own test.

Non-blocking, but as a possible follow-up: checkUnsupportedOperationsForIFSTIndex's three UnsupportedOperationException branches (non-STRING type, no dictionary, multi-value column) aren't exercised here — might be worth a couple more cases, though I see FSTIndexHandler doesn't test those either, so happy to leave that for a separate pass.

[gortiz]

Logic in the new tests checks out against IFSTIndexHandler's actual branches (verified needUpdateIndices/updateIndices paths and BaseIndexHandler's normalization). Low-risk, test-only addition. Left a couple of non-blocking style suggestions in a separate comment.

[Akanksha-kedia]

@xiangfu0 @gortiz — would appreciate a review when you get a chance 🙏

[Jackie-Jiang]

Is there existing test for FSTIndexHandler? I'm not able to find any