Jackie-Jiang commented on code in PR #19072:
URL: https://github.com/apache/pinot/pull/19072#discussion_r3648895506
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/StandardIndexes.java:
##########
@@ -97,8 +98,8 @@ public static IndexType<ForwardIndexConfig,
ForwardIndexReader, ForwardIndexCrea
IndexService.getInstance().get(DICTIONARY_ID);
}
- public static IndexType<IndexConfig, NullValueVectorReader, ?>
nullValueVector() {
- return (IndexType<IndexConfig, NullValueVectorReader, ?>)
+ public static IndexType<NullValueVectorConfig, NullValueVectorReader, ?>
nullValueVector() {
+ return (IndexType<NullValueVectorConfig, NullValueVectorReader, ?>)
IndexService.getInstance().get(NULL_VALUE_VECTOR_ID);
}
Review Comment:
Good catch on the generic-invariance point in principle, but I checked and
it is a no-op here, so I'd like to keep the narrowed type.
- **No source break in practice.** Grepping both `apache/pinot` and our
downstream repo for `IndexType<IndexConfig, NullValueVectorReader, ...>`
returns zero hits. Every one of the ~40 `StandardIndexes.nullValueVector()`
call sites passes it to a wildcard-accepting API (`hasIndexFor`, `getIndex`,
`newIndexFor`, `columnsWithIndexEnabled`, `FieldIndexConfigs.Builder#with`,
Mockito `eq(...)`, set membership) or to `FieldIndexConfigs#getConfig`, which
infers `C` from the index type. The only site that named the type explicitly
was this index's own `ReaderFactory`, updated in this PR.
- **It aligns the accessor with every sibling.** `bloomFilter()` →
`BloomFilterConfig`, `json()` → `JsonIndexConfig`, `forward()` →
`ForwardIndexConfig`, `h3()` → `H3IndexConfig`, and so on. `nullValueVector()`
returned the generic `IndexConfig` only because it had no dedicated config
class; now that it does, this brings it into line rather than out of it.
- **The suggested alternative is worse.** Keeping `IndexConfig` and casting
at call sites would make the declared type contradict the actually-registered
`AbstractIndexType<NullValueVectorConfig, ...>` and force an unchecked cast in
the handler to read `backfill`.
On documenting it: the PR description carries a rolling-upgrade note, since
the more consequential compatibility point is that once a column opts in via
`indexes.null`, a node still running the pre-change code fails to resolve that
table's index configs (the old deserializer treats `indexes.null` as an
exclusive alternative to the always-present derived config).
--
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]
