Add unit tests for JsonIndexHandler index lifecycle

[Akanksha-kedia]

Summary

JsonIndexHandler had no unit test coverage. Adds JsonIndexHandlerTest 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

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.12%. Comparing base (bcf3f66) to head (9baa871).
⚠️ Report is 55 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18962      +/-   ##
============================================
+ Coverage     64.86%   65.12%   +0.26%     
- Complexity     1347     1403      +56     
============================================
  Files          3392     3407      +15     
  Lines        211663   213087    +1424     
  Branches      33306    33606     +300     
============================================
+ Hits         137290   138770    +1480     
+ Misses        63296    63127     -169     
- Partials      11077    11190     +113     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (?)
java-21	65.12% <ø> (+0.26%)	⬆️
temurin	65.12% <ø> (+0.26%)	⬆️
unittests	65.12% <ø> (+0.26%)	⬆️
unittests1	56.85% <ø> (-0.14%)	⬇️
unittests2	37.51% <ø> (+0.28%)	⬆️

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.

[Akanksha-kedia]

cc @xiangfu0 @J-HowHuang @Jackie-Jiang

[Jackie-Jiang]

The newly added test is failing. Please take a look

Please fix the tests

[Copilot]

Pull request overview

Adds focused unit test coverage for JsonIndexHandler in pinot-segment-local, exercising the handler’s “should we rebuild?” decision logic and the index-removal path during updates.

Changes:

• Introduces JsonIndexHandlerTest covering needUpdateIndices() for: dropped-config columns, newly-configured columns (metadata present/absent), and already-up-to-date indexes.
• Adds a unit test for updateIndices() verifying JSON index removal when the column is no longer configured.

[Akanksha-kedia]

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

[gortiz]

Thanks for adding coverage here — JsonIndexHandler genuinely had none before this.

On the mocking approach: this follows existing precedent well. VectorIndexHandlerTest (same package) tests the identical needUpdateIndices/updateIndices decision logic with the same Mockito pattern (mock(SegmentDirectory.class), mock(SegmentMetadataImpl.class), mocked Reader/Writer), so this isn't introducing a new style — it's consistent with how this specific kind of lifecycle-dispatch logic is tested elsewhere in the module.

One gap worth flagging: all 5 tests exercise the add/remove decision logic, but none exercise the actual index-creation path (createJsonIndexForColumn, handleDictionaryBasedColumn/handleNonDictionaryBasedColumn — forward-index rebuild, marker-file crash recovery, dictionary vs. non-dictionary handling, v3 single-file writing). That's the riskiest part of this class and it's still untested. Testing it properly would need a real on-disk segment rather than more mocks (see ForwardIndexHandlerTest's approach with SegmentGeneratorConfig + SegmentIndexCreationDriverImpl + SegmentLocalFSDirectory) — mocking the index-creator/reader chain would just replace one set of untested assumptions with another. Would be a great follow-up PR if not in scope here.

Minor nit: testNeedUpdateReturnsTrueWhenColumnRemovedFromConfig passes Map.of() and relies on BaseIndexHandler's constructor auto-filling missing columns with FieldIndexConfigs.EMPTY when the keyset doesn't match segmentMetadata.getAllColumns(). It's correct, but the intent would read more clearly if the config for COLUMN were built explicitly (e.g. a FieldIndexConfigs without json enabled) rather than via that implicit fallback.

[Akanksha-kedia]

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