Follow-up fixes from PR #18643 (OPEN_STRUCT storage layer) review comments - #19093
Open
tarun11Mavani wants to merge 3 commits into
Open
Follow-up fixes from PR #18643 (OPEN_STRUCT storage layer) review comments#19093tarun11Mavani wants to merge 3 commits into
tarun11Mavani wants to merge 3 commits into
Conversation
Review-comment fixes from the storage PR (apache#18643) that were not included in the merged version: - OpenStructColumnSplitter: computeIfAbsent instead of containsKey+put, Utf8.encodedLength instead of getBytes(UTF_8).length - MutableOpenStructIndex: volatile copy-on-write Javadoc clarification, pass PinotDataType directly to tryCoerce instead of converting per call - MutableKeyColumn: cache PinotDataType destType to avoid repeated ColumnDataType.fromDataTypeSV().toPinotDataType() conversion - MutableOpenStructDataSource: snapshot keyColumns map before iterating - MutableSegmentImpl: remove unused getOpenStructIndex() method - IndexLoadingConfig: persist OPEN_STRUCT segment metadata so child configs survive refreshIndexConfigs() dirty rebuilds
…umns - ImmutableSegmentImpl: warn log when an OPEN_STRUCT parent's schema/fieldSpec is missing or mismatched, so dense/sparse child data silently becomes unqueryable - MutableSegmentImpl: checkState guard requiring the open_struct_index to be enabled when building a DataSource for an OPEN_STRUCT column - ImmutableOpenStructDataSource: getMapValue()/readValue() to reconstruct the full key-value map (dense + sparse) for a doc, used by the seal path and minion tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19093 +/- ##
============================================
+ Coverage 65.45% 65.50% +0.05%
Complexity 1421 1421
============================================
Files 3428 3430 +2
Lines 216598 218039 +1441
Branches 34311 34657 +346
============================================
+ Hits 141778 142835 +1057
- Misses 63405 63638 +233
- Partials 11415 11566 +151
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:
|
writeSparseJsonColumn() hand-built a partial PropertiesConfiguration for the OPEN_STRUCT sparse ($__sparse__) column, hard-coding a subset of keys instead of routing through the standard metadata writer. This branch's ColumnMetadataImpl still tolerates missing bitsPerElement/ lengthOfEachEntry/maxNumberOfMultiValues via UNAVAILABLE defaults, so it wasn't fatal here, but the metadata was still wrong (e.g. CARDINALITY was approximated as a non-null doc count rather than the real distinct value count), and a sibling branch's stricter reader throws NoSuchElementException on this exact gap. Route the sparse column through the same AbstractColumnStatisticsCollector + BaseSegmentCreator.addColumnMetadataInfo() path that writeDenseKeyColumn() already uses, instead of a hand-rolled subset of keys that had drifted out of sync with what the reader expects. Extend testSparseJsonColumnWritten to round-trip the produced properties through ColumnMetadataImpl.fromPropertiesConfiguration() as a regression check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up fixes from PR #18643 (OPEN_STRUCT storage layer) review comments.
Review-comment fixes:
OpenStructColumnSplitter: usecomputeIfAbsentinstead of get+put;Utf8.encodedLengthinstead of allocatingbyte[]for length measurementMutableOpenStructIndex: expanded volatile copy-on-write Javadoc;tryCoercetakesPinotDataTypedirectly (avoids repeated conversion per ingested value)MutableKeyColumn: cachePinotDataType destTypeat constructionMutableOpenStructDataSource: snapshotkeyColumnsbefore iteration (concurrent-modification guard)MutableSegmentImpl: remove deadgetOpenStructIndex()methodIndexLoadingConfig: persist_openStructSegmentMetadataso child configs surviverefreshIndexConfigs()dirty rebuildsSegment-lifecycle hardening:
ImmutableOpenStructDataSource: addgetMapValue(docId)— reconstructs the original map from dense per-key DataSources + sparse JSON; used byPinotSegmentRecordReaderandSegmentColumnarIndexCreatorImmutableSegmentImpl: warn log when skipping OPEN_STRUCT parent due to missing/wrong FieldSpecMutableSegmentImpl:Preconditions.checkStateguard intoDataSource()for OPEN_STRUCT with disabled indexTest plan
ImmutableOpenStructDataSourceTest— 5 new tests forgetMapValue(): dense-only, null doc, dense+sparse merge, sparse-only null, empty segment