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#18920Akanksha-kedia wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18920 +/- ##
============================================
+ Coverage 65.05% 65.22% +0.16%
- Complexity 1403 1405 +2
============================================
Files 3399 3418 +19
Lines 212808 214708 +1900
Branches 33568 33937 +369
============================================
+ Hits 138443 140043 +1600
- Misses 63223 63364 +141
- Partials 11142 11301 +159
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found a high-signal stale-index issue; see inline comment.
| // 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)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Addressed @xiangfu0's review feedback: changed the |
|
Fix: MAP column JSON re-indexing (commit a10a419) The two CI failures (unit + integration Root cause: When the config-change or legacy-upgrade rebuild was triggered for a MAP column that already had a JSON index, Fix: For MAP columns, use All 10 |
|
The only failing check is Pinot Binary Compatibility Check, which is pre-existing and unrelated to this PR. The failure is in This PR only touches |
…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
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>
…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>
d6b3c9c to
1d251ea
Compare
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-severity issue; see inline comment.
| // treat as a one-time rebuild to backfill the stored config and catch any config drift. | ||
| JsonIndexConfig currentConfig = _jsonIndexConfigs.get(column); | ||
| JsonIndexConfig storedConfig = readStoredJsonIndexConfig(column, properties); | ||
| if (properties != null && (storedConfig == null || !storedConfig.equals(currentConfig))) { |
There was a problem hiding this comment.
Treating storedConfig == null as a rebuild forces every pre-metadata JSON index through createJsonIndexForColumn(). Existing segments can have a JSON index on a forward-index-disabled column without the dictionary/inverted pair required by createForwardIndexIfNeeded() because JsonIndexType.requiresDictionary() is false. Those segments loaded before, but this path now removes the existing JSON index and then fails reload even when table config did not change. Please only backfill this metadata when the forward index is present/reconstructable, or preserve the existing index and require refresh/backfill for this legacy case.
|
@xiangfu0 @shounakmk219 would appreciate a review when you get a chance. This PR makes |
…ility JsonIndexHandler previously treated storedConfig == null (index predates config persistence) as an unconditional rebuild trigger. Segments that have a JSON index on a forward-index-disabled column with no dictionary/inverted pair cannot call createForwardIndexIfNeeded() — the Preconditions.checkState inside BaseIndexHandler throws with 'must have a dictionary to regenerate'. Fix: introduce isForwardIndexAvailable() and use it to guard the legacy- backfill path. Only trigger rebuild when the forward index already exists or can be reconstructed from dictionary + inverted index. When the forward index is absent and unrecoverable, the existing JSON index is preserved untouched and no rebuild is attempted. The configChanged path (storedConfig != null && !equal) is unchanged: that path was pre-existing and requires an explicit segment refresh when the forward index is unrecoverable. Tests: split the single legacy-segment test into three cases: - legacy with forward index present → rebuild (backfill) - legacy with forward index absent and no dict/inverted → no rebuild (preserve) - legacy with no forward index but dict+inverted reconstructable → rebuild
3 participants
Summary
Closes #10506.
JsonIndexHandler: After creating a JSON index, the active
JsonIndexConfigis serialised to JSON and stored inmetadata.propertiesundercolumn.<name>.jsonIndexConfig. On the next segment reloadneedUpdateIndices()reads the stored config, compares it with the current table config usingequals(), 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
_indexPathsand_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.propertiesis loaded once perneedUpdateIndices()/updateIndices()call; the persist step inupdateIndices()batches allsetPropertycalls 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 persistedjsonIndexConfig(built before this feature). After an upgrade, changingmaxLevels,includePaths, etc. could leave the old index in place and produce stalejson_matchresults.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→testNeedUpdateReturnsTrueWhenNoStoredConfigwith assertion flipped toassertTrue— regression test for the legacy-index case.Test plan
JsonIndexHandlerTest(5 unit tests):maxLevelschangesexcludeArraychangesmaxBytesSizechanges (would have silently failed before theequalsfix)Existing
SegmentPreProcessorTestJSON index tests continue to pass unmodifiedcc @Jackie-Jiang @xiangfu0 @yashmayya