xiangfu0 commented on PR #19955: URL: https://github.com/apache/pinot/pull/19955#issuecomment-6330992736
**Overall:** Nice, well-scoped fix. The root cause is accurate: `ValueReader` was reading the source forward index without its null vector, so a null source doc surfaced as the stored default-value placeholder and the transform ran on that placeholder instead of on `null`. I traced the full chain and it holds together: - `InbuiltFunctionEvaluator` already propagates `null -> null` for functions without nullable parameters (the `hasNullableParameters()` check), so feeding real `null` makes `times(null, 2)` return `null` exactly as at ingestion. - The returned `null` then flows into the existing derived-null path (`outputValue == null` -> default value + `nullValueVectorCreator.setNull(i)`), so the derived column's null vector lines up with the source. - When null handling is disabled there is no source null vector, `isNull` stays `false`, and behavior is unchanged. I ran the new test locally (JDK 25): it passes on both v1 and v3 with the fix, and fails on the pre-fix code with exactly `Derived value must be null where the source is null, doc: 1`. Good regression coverage, and the read pattern matches existing usage in `StarTreeLoaderUtils` / `NullValueIndexType`. Only minor/non-blocking notes inline. **Approving.** -- 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]
