xiangfu0 commented on PR #18504: URL: https://github.com/apache/pinot/pull/18504#issuecomment-4457164243
Thanks @raghavyadav01! Addressed both in bf6327d21e: **1. Default `BlockValSet.isDictionaryEncoded()` returning `getDictionary() != null`** — kept the default for SPI compat (changing it to abstract would break any external implementer) but strengthened the Javadoc to call out the trap explicitly and added explicit overrides to every in-tree impl: `TransformBlockValSet`, `RowBasedBlockValSet`, `FilteredRowBasedBlockValSet`, `DataBlockValSet`, `FilteredDataBlockValSet`. So no in-tree caller relies on the default anymore — only future external impls would, and the Javadoc now warns them. **2. `ColumnContext.fromTransformFunction` keeping `dictionary != null`** — added a regression test `testDistinctOnTransformOfRawDictColumnReturnsSameResults` that runs `SELECT DISTINCT UPPER(rawDictDim) FROM t`, which goes through `TransformBlockValSet`. Confirmed it passes. The invariant the comment relies on (transforms that expose a dictionary always built it themselves) is enforced at the source: `IdentifierTransformFunction.java:50-51` and `ItemTransformFunction.java:64-68` already hide the dictionary on RAW+dictionary columns — so any composite transform built on top inherits the safety. -- 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]
