Copilot commented on code in PR #19072:
URL: https://github.com/apache/pinot/pull/19072#discussion_r3642169387
##########
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:
Changing this helper’s generic return type from `IndexConfig` to
`NullValueVectorConfig` is source-incompatible for any downstream code that
typed the result as `IndexType<IndexConfig, ...>` (generic invariance), even
though it’s likely binary-compatible due to type erasure. If
`pinot-segment-spi` is treated as an external surface, consider whether you
want to preserve source compatibility (e.g., keep the old signature and cast at
call sites, or introduce a separate typed accessor) and document the change
accordingly.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexType.java:
##########
@@ -110,20 +116,53 @@ protected IndexReaderFactory<NullValueVectorReader>
createReaderFactory() {
return ReaderFactory.INSTANCE;
}
+ @Override
+ public void validate(FieldIndexConfigs indexConfigs, FieldSpec fieldSpec,
TableConfig tableConfig) {
+ if (indexConfigs.getConfig(this).isBackfill()) {
+ // Backfill reconstructs nulls by comparing each stored value against
the column's default null value, which is
+ // only meaningful for scalar stored types. MAP (and other complex
types) are not supported because:
+ // - the default null value for a MAP is an empty map — an ordinary
value rather than a rare sentinel — so
+ // treating every empty map as null would be far too lossy to be
safe; and
+ // - an OPEN_STRUCT-backed MAP is materialized into child columns with
no single scannable parent forward
+ // index, so there is nothing coherent to scan for the parent column.
+ // TODO: Revisit MAP/complex backfill if complex-type null handling
matures and a safe (non-occurring) sentinel
+ // default null value becomes available.
Review Comment:
Avoid leaving an untracked TODO in production code. Either remove it or
replace it with a concrete link to an issue/design doc so it doesn’t become
stale (Pinot’s agent guidelines call out TODO hygiene).
--
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]
