Jackie-Jiang commented on code in PR #19996:
URL: https://github.com/apache/pinot/pull/19996#discussion_r4352276383
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java:
##########
@@ -188,6 +189,27 @@ boolean isNull(int docId)
Object getValue(int docId)
throws IOException;
+ /// Applies the supplied transformer to the value at the given document id.
By default, returns
+ /// `transformer.transform(getValue(docId))`, passing null values to the
transformer as well.
+ /// The default invokes [ColumnTransformer#transform(Object)] even when
[ColumnTransformer#isNoOp()] returns `true`.
+ ///
+ /// Callers must supply a deterministic transformer: repeated
transformations of the same value must produce
+ /// equivalent results. Callers must not depend on invocation counts or
transformer side effects.
+ ///
+ /// Readers may override this method to avoid materializing repeated raw
values, reuse immutable conversion results,
+ /// or skip no-op transformations. Overrides must preserve the conversion
result, including nulls and conversion
+ /// failures. This method does not change [#getValue(int)] or the typed
accessors.
+ /// Callers remain responsible for applying this conversion in the correct
order relative to other transformations.
+ ///
+ /// @param docId 0-based document id, from 0 (inclusive) to
[#getTotalDocs()] (exclusive)
+ /// @param transformer deterministic transformation to apply to the value
+ /// @throws IOException if an I/O error occurs while reading
+ @Nullable
+ default Object getTransformedValue(int docId, ColumnTransformer transformer)
+ throws IOException {
+ return transformer.transform(getValue(docId));
Review Comment:
Can we filter no-op transformers in the caller during pipeline setup and
assert that precondition here?
```java
assert !transformer.isNoOp() : "No-op transformers must be filtered by the
caller";
return transformer.transform(getValue(docId));
```
With assertions disabled, this avoids evaluating `isNoOp()` for each
document. Please document that callers must supply a non-no-op transformer and
remove the statements allowing no-op inputs or overrides to skip them. An
assertion-enabled test can cover rejection.
This matters for existing implementations: `SanitizationColumnTransformer`
for an INT column reports `isNoOp() == true`, but calling `transform(42)`
dereferences null configuration. Making these inputs invalid for this API keeps
that handling in the caller.
--
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]
