Rebuild JSON index when JsonIndexConfig changes; fix equals/hashCode to include all fields by Akanksha-kedia · Pull Request #18920 · apache/pinot

Akanksha-kedia · 2026-07-06T10:07:14Z

Summary

Closes #10506.

JsonIndexHandler: After creating a JSON index, the active JsonIndexConfig is serialised to JSON and stored in metadata.properties under column.<name>.jsonIndexConfig. On the next segment reload needUpdateIndices() reads the stored config, compares it with the current table config using equals(), and triggers a rebuild when they differ. Legacy segments (indexed before this change, no stored config present) are now treated as a one-time rebuild case — the index is rebuilt to backfill the stored config and catch any config drift that may have occurred during an upgrade.
JsonIndexConfig.equals / hashCode: Fixed to include _indexPaths and _maxBytesSize, which were previously missing. Without this fix, config changes to those two fields would be invisible to any equality-based comparison (including the new detection logic).
Performance: metadata.properties is loaded once per needUpdateIndices() / updateIndices() call; the persist step in updateIndices() batches all setProperty calls and writes the file once at the end rather than once per rebuilt column.

Review changes (v2)

Addressed @xiangfu0's feedback:

Fixed legacy-segment handling: The original guard storedConfig != null && !storedConfig.equals(currentConfig) silently skipped segments that had a JSON index but no persisted jsonIndexConfig (built before this feature). After an upgrade, changing maxLevels, includePaths, etc. could leave the old index in place and produce stale json_match results.

Changed to properties != null && (storedConfig == null || !storedConfig.equals(currentConfig)): when metadata is readable (properties != null) but no stored config is found (storedConfig == null), the segment is treated as a one-time rebuild/backfill case. In-memory segments (properties == null) continue to skip the check.
Updated unit test: testNeedUpdateReturnsFalseWhenNoStoredConfig → testNeedUpdateReturnsTrueWhenNoStoredConfig with assertion flipped to assertTrue — regression test for the legacy-index case.

Test plan

JsonIndexHandlerTest (5 unit tests):
- No rebuild when stored config equals current config
- Rebuild triggered when maxLevels changes
- Rebuild triggered when excludeArray changes
- Rebuild triggered when maxBytesSize changes (would have silently failed before the equals fix)
- Rebuild triggered for legacy segments with no stored config (one-time backfill)
Existing SegmentPreProcessorTest JSON index tests continue to pass unmodified

cc @Jackie-Jiang @xiangfu0 @yashmayya

…clude all fields JsonIndexHandler now persists the active JsonIndexConfig into segment metadata (column.<name>.jsonIndexConfig) after building a JSON index. On reload, needUpdateIndices() compares the stored config against the current table config and triggers a rebuild when they differ. Segments indexed before this change carry no stored config and are treated as unchanged (backward compatibility). Also fixes JsonIndexConfig.equals/hashCode to include _indexPaths and _maxBytesSize, which were previously silently excluded, causing config changes to those fields to be invisible to any equality-based comparison. Resolves: apache#10506

codecov-commenter · 2026-07-06T11:09:06Z

Codecov Report

❌ Patch coverage is 62.96296% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.88%. Comparing base (bcf3f66) to head (a10a419).
⚠️ Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
...t/index/loader/invertedindex/JsonIndexHandler.java	64.70%	12 Missing and 6 partials ⚠️
...apache/pinot/spi/config/table/JsonIndexConfig.java	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18920      +/-   ##
============================================
+ Coverage     64.86%   64.88%   +0.02%     
  Complexity     1347     1347              
============================================
  Files          3392     3398       +6     
  Lines        211663   212602     +939     
  Branches      33306    33513     +207     
============================================
+ Hits         137290   137955     +665     
- Misses        63296    63499     +203     
- Partials      11077    11148      +71

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (?)`
java-21	`64.88% <62.96%> (+0.02%)`	⬆️
temurin	`64.88% <62.96%> (+0.02%)`	⬆️
unittests	`64.88% <62.96%> (+0.02%)`	⬆️
unittests1	`56.89% <46.29%> (-0.10%)`	⬇️
unittests2	`37.25% <50.00%> (+0.03%)`	⬆️

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.

xiangfu0

Found a high-signal stale-index issue; see inline comment.

xiangfu0 · 2026-07-07T00:53:39Z

+      // Column exists in both existing indexes and config; check if config changed.
+      JsonIndexConfig currentConfig = _jsonIndexConfigs.get(column);
+      JsonIndexConfig storedConfig = readStoredJsonIndexConfig(column, properties);
+      if (storedConfig != null && !storedConfig.equals(currentConfig)) {


This skips legacy segments that already have a JSON index but no persisted jsonIndexConfig. Because needProcess() is gated on this result, those segments never rebuild or backfill metadata; after upgrading, changing maxLevels/includePaths/etc. can leave the old JSON index in place and produce stale json_match results. Treat missing stored config as a one-time rebuild/backfill case and cover legacy-index plus config-change in a regression test.

Fixed. The condition is now properties != null && (storedConfig == null || !storedConfig.equals(currentConfig)).

properties == null (in-memory segment) → skip rebuild, same as before.

properties != null && storedConfig == null (legacy segment with no persisted config) → trigger a one-time rebuild to backfill the stored config and catch any config drift.

properties != null && !storedConfig.equals(currentConfig) (config changed) → rebuild as before.

The test testNeedUpdateReturnsTrueWhenNoStoredConfig was also updated to assert true for legacy segments.

Treat null storedConfig (legacy segment, pre-config-persistence) as a one-time rebuild trigger rather than silently skipping. Without this fix, segments with an existing JSON index but no persisted jsonIndexConfig were never rebuilt after config changes (maxLevels, includePaths, etc.), leaving stale json_match results after an upgrade. Changed condition from `storedConfig != null && !storedConfig.equals(...)` to `properties != null && (storedConfig == null || !storedConfig.equals(...))` in both needUpdateIndices() and updateIndices(). When properties is null (in-memory segment) the check is still skipped entirely. Updated test to assert TRUE for the legacy-segment case (regression test for the reported issue). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Akanksha-kedia · 2026-07-07T04:56:51Z

Addressed @xiangfu0's review feedback: changed the storedConfig != null && ... guard to properties != null && (storedConfig == null || !storedConfig.equals(currentConfig)) in both needUpdateIndices and updateIndices. Legacy segments (no persisted jsonIndexConfig) now trigger a one-time rebuild to backfill config and catch any drift. Updated the unit test to assert true for the legacy-segment case. Ready for re-review.

…tring() When JsonIndexHandler re-builds a JSON index for an existing MAP column (triggered by the config-change or legacy-upgrade rebuild), handleNon DictionaryBasedColumn was calling forwardIndexReader.getString() on MAP column values. MAP columns store binary-encoded data (via MapUtils. serializeMap), not UTF-8 text; decoding those bytes as UTF-8 produces garbage strings containing null bytes (CTRL-CHAR code 0), which cause JsonParseException: Illegal character (CTRL-CHAR, code 0) when the JSON index creator tries to index them. Fix: detect MAP data type and call getMap() + MapUtils.toString() instead, consistent with how ForwardIndexReader.getStrings(int[], ...) already handles MAP in its bulk-read path (line 427 of ForwardIndexReader.java). Fixes failing TableIndexingTest cases for all *_map_*,json_index combinations introduced by the legacy-rebuild trigger in the previous commit. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Akanksha-kedia · 2026-07-08T06:01:36Z

Fix: MAP column JSON re-indexing (commit a10a419)

The two CI failures (unit + integration TableIndexingTest.*_map_*,json_index) were caused by a bug in handleNonDictionaryBasedColumn that this new commit fixes.

Root cause: When the config-change or legacy-upgrade rebuild was triggered for a MAP column that already had a JSON index, handleNonDictionaryBasedColumn called forwardIndexReader.getString(docId, ctx) on the MAP forward index. MAP columns are stored as binary-encoded data (via MapUtils.serializeMap), not UTF-8 text. Decoding those bytes as UTF-8 produces garbage strings containing CTRL-CHAR(0) null bytes, which caused:

JsonParseException: Illegal character (CTRL-CHAR, code 0): only regular white space (\r, \n, \t) is allowed between tokens

Fix: For MAP columns, use forwardIndexReader.getMap(docId, ctx) + MapUtils.toString() instead of getString(), consistent with how ForwardIndexReader.getStrings(int[], ...) already handles the MAP type in its bulk-read path.

All 10 *_map_*,json_index test cases should now pass.

Akanksha-kedia · 2026-07-08T13:06:02Z

The only failing check is Pinot Binary Compatibility Check, which is pre-existing and unrelated to this PR.

The failure is in pinot-spi — methods removed from org.apache.pinot.spi.data.readers.ColumnReader (hasNext(), nextInt(), nextLong(), etc.) and PinotDataType.toUuidBytesArray(). These are upstream changes by other contributors that already break binary compat on master.

This PR only touches pinot-segment-local (JSON index rebuild logic and JsonIndexConfig equals/hashCode). All other checks pass: Linter ✅, Unit Tests ✅, Integration Tests ✅, Quickstart ✅, Compatibility Regression ✅.

xiangfu0 reviewed Jul 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rebuild JSON index when JsonIndexConfig changes; fix equals/hashCode to include all fields#18920

Rebuild JSON index when JsonIndexConfig changes; fix equals/hashCode to include all fields#18920
Akanksha-kedia wants to merge 3 commits into
apache:masterfrom
Akanksha-kedia:fix/json-index-config-rebuild

Akanksha-kedia commented Jul 6, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 6, 2026 •

edited

Loading

Uh oh!

xiangfu0 left a comment

Uh oh!

xiangfu0 Jul 7, 2026

Uh oh!

Akanksha-kedia Jul 7, 2026

Uh oh!

Akanksha-kedia commented Jul 7, 2026

Uh oh!

Akanksha-kedia commented Jul 8, 2026

Uh oh!

Akanksha-kedia commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Akanksha-kedia commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review changes (v2)

Test plan

Uh oh!

codecov-commenter commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Akanksha-kedia Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Akanksha-kedia commented Jul 7, 2026

Uh oh!

Akanksha-kedia commented Jul 8, 2026

Uh oh!

Akanksha-kedia commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Akanksha-kedia commented Jul 6, 2026 •

edited

Loading

codecov-commenter commented Jul 6, 2026 •

edited

Loading