raghavyadav01 commented on PR #18504:
URL: https://github.com/apache/pinot/pull/18504#issuecomment-4456427818
Two questions on the design — not blockers, just want to confirm intent:
**1. Default `BlockValSet.isDictionaryEncoded()` returns `getDictionary() !=
null`.**
```java
default boolean isDictionaryEncoded() {
return getDictionary() != null;
}
```
This is exactly the pattern the PR is fixing at the projection layer. It's
correct today (every existing non-projection BVS couples the two), but any
future BVS impl that decouples dictionary presence from dict-id readability
will silently regress every call site. Should the default be `false` (or
abstract) to force explicit overrides, or do we rely on the Javadoc contract?
**2. `ColumnContext.fromTransformFunction` keeps the old `dictionary !=
null` semantics:**
```java
return new ColumnContext(..., dictionary, dictionary != null, null);
```
Comment says transforms always self-build their dictionary. Plausible for
current transforms, but it's the same assumption that bit us at the column
layer. No test covers `SELECT DISTINCT someTransform(rawDictCol)`. Worth a
regression test for the transform path, or do we trust the invariant?
--
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]
