gortiz commented on PR #19349: URL: https://github.com/apache/pinot/pull/19349#issuecomment-5453899773
Good point — I'd like to check we're targeting the same scenario, because I think there are two. What this PR fixes: the star-tree config is already **gone** from the table config (or `enableDynamicStarTreeCreation` is false, so it's never reconciled), the column moves to `noDictionaryColumns`, and the segment isn't re-pushed. `ForwardIndexHandler` drops the dictionary and the stale star-tree in the segment becomes unreadable — `StarTreeLoaderUtils` takes the bit width from the main `ColumnMetadata`, which is now `UNAVAILABLE`, and the whole segment load fails. There's no star-tree config left to key "keep the dictionary" off, and for segments already converted on disk the dictionary is gone regardless, so those need a repair path either way. The scenario your comment describes — star-tree still configured on the column while the column moves to `noDictionaryColumns` — is rejected upfront today by `TableConfigUtils#validateStarTreeIndexConfigs`: _"Cannot create star-tree index on dimension column: %s without dictionary"_. So it's only reachable with validation skipped. Are you thinking of that case (and of relaxing the validation now that #19153 makes RAW forward + separated dictionary viable), or of the config-dropped case? If the former, I agree it's worth doing and it looks tractable: `ForwardIndexHandler#computeOperations` already preserves the dictionary through a DICT→RAW flip via `dictRequiredByIndex = DictionaryIndexConfig.requiresDictionary(...)`, so it's mostly a matter of making a star-tree dimension count as "requires dictionary". The wrinkle is that star-tree configs are table-level and not reachable from the per-column `FieldIndexConfigs` the handler sees. I'd prefer it as a follow-up on top of this PR, since it's additive and doesn't remove the need for this repair path — happy to take it if you agree on the shape. On the two inline questions about #19153: the check here is on the **dictionary**, not on the forward-index encoding, so a RAW-forward + separated-dictionary dimension is never flagged. `BaseSegmentCreator` writes `hasDictionary = (dictionaryCreator != null)` independently of `FORWARD_INDEX_ENCODING` and writes `BITS_PER_ELEMENT` from cardinality regardless; `ColumnMetadataImpl` only forces `bitsPerElement = UNAVAILABLE` when `hasDictionary` is false. I've pushed `testStarTreeDimensionConvertedToRawWithSeparatedDictionary` to confirm: with the star-tree config kept and the dimension switched to `encodingType: RAW` + dictionary index enabled, pre-processing flips the forward index, keeps the dictionary, and leaves the star-tree in place — the loaded segment still serves it, with a live dictionary on the column. No rebuild needed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
