Repository navigation
Conversation
Addresses four problems: 1) Provide PhysicalSegmentColumnInspector from PartialQueryableIndexSegment. Previously it was not provided, so segmentMetadata queries were not able to report types. 2) Look at all cluster groups when determining capabilities, not just the first one. (They can differ in the details, like auto-determined type and hasMultipleValues.) 3) Use the DimensionIndexer's ColumnFormat when writing columns in cluster groups and projections. Previously the ColumnFormat present at creation time was used, which could differ in details (such as hasMultipleValues). This caused multi-valued columns to be incorrectly persisted, in situations where the persist logic did not realize they were multi-value. 4) Updates Capable.or and Capable.and to use correct 3-valued logic. This made certain new code simpler to write, and as far as I can tell, will only have positive effects on existing callers.
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
Correct the fully downloaded inspector capabilities before merging so string metadata analysis also works when bitmap indexes are disabled. The capability combination and persistence changes otherwise address the multi-value and cross-group cases inspected.
Reviewed 38 of 38 changed files (22 production files and 16 test files); findings in 1 file. This was a static review, including adjacent selectors, serialization, metadata analysis, and capability consumers.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| || PhysicalSegmentInspector.class.equals(clazz)) { | ||
| if (index.isFullyDownloaded()) { | ||
| // If fully downloaded, we can return some additional information. | ||
| return (T) new QueryableIndexPhysicalSegmentInspector(index); |
There was a problem hiding this comment.
[P2] Use resident column capabilities for the full-download inspector
Finding: For a non-clustered V10 segment with a string dimension configured with createBitmapIndex=false, this full-download branch still reports isDictionaryEncoded=false: QueryableIndexPhysicalSegmentInspector.getColumnCapabilities delegates to PartialQueryableIndex.getColumnCapabilities, which always builds descriptor-only capabilities. SegmentAnalyzer finds the resident column holder, but its string-analysis path has no DictionaryEncodedStringValueIndex without bitmaps and only falls back to the column dictionary when these capabilities report dictionary encoding. Consequently a segment metadata query requesting CARDINALITY or MINMAX returns cardinality 0 and null bounds for a populated dimension even after every file has been downloaded. The added analysis test uses a bitmap-indexed dimension, so it does not exercise this path.
Suggestion: Have the fully downloaded inspector obtain non-clustered column capabilities from the resident holders, and cover string metadata analysis with bitmap indexes disabled.
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
Safe to merge. The updated head resolves the prior capability-inspector issue and adds coverage for fully downloaded segments without bitmap indexes. I found no additional PR-caused correctness or regression issues.
Reviewed 39 of 39 changed files, beginning with the supplied incremental diff and then checking the full PR diff, surrounding capability handling, clustered cursors, projection merging, incremental indexes, and tests.
This is an automated review by Codex GPT-5.6-Luna(max)
Addresses four problems:
Provide PhysicalSegmentColumnInspector from PartialQueryableIndexSegment.
Previously it was not provided, so segmentMetadata queries were not
able to report types.
Look at all cluster groups when determining capabilities, not just
the first one. (They can differ in the details, like auto-determined
type and hasMultipleValues.)
Use the DimensionIndexer's ColumnFormat when writing columns in
cluster groups and projections. Previously the ColumnFormat present
at creation time was used, which could differ in details (such as
hasMultipleValues). This caused multi-valued columns to be
incorrectly persisted, in situations where the persist logic did
not realize they were multi-value.
Updates Capable.or and Capable.and to use correct 3-valued logic.
This made certain new code simpler to write, and as far as I can
tell, will only have positive effects on existing callers.