xiangfu0 commented on code in PR #19996:
URL: https://github.com/apache/pinot/pull/19996#discussion_r4347022669
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java:
##########
@@ -188,6 +189,23 @@ boolean isNull(int docId)
Object getValue(int docId)
throws IOException;
+ /// Applies the supplied deterministic transformer to the value at the given
document id. By default, returns
+ /// `transformer.transform(getValue(docId))`, passing null values to the
transformer as well.
+ ///
+ /// Readers may override this method to avoid materializing repeated raw
values or to reuse immutable conversion
+ /// results. Overrides must preserve the conversion result, including nulls
and conversion failures; callers must not
+ /// depend on the converter's invocation count. 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
Review Comment:
The doc calls the transformer `deterministic`, but nothing enforces it and
the default works either way. Consider phrasing it as a requirement on
callers/overrides so it reads as a contract rather than an assumption.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java:
##########
@@ -188,6 +189,23 @@ boolean isNull(int docId)
Object getValue(int docId)
throws IOException;
+ /// Applies the supplied deterministic transformer to the value at the given
document id. By default, returns
+ /// `transformer.transform(getValue(docId))`, passing null values to the
transformer as well.
+ ///
+ /// Readers may override this method to avoid materializing repeated raw
values or to reuse immutable conversion
+ /// results. Overrides must preserve the conversion result, including nulls
and conversion failures; callers must not
+ /// depend on the converter's invocation count. 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)
Review Comment:
Since `ColumnTransformer` exposes `isNoOp()`, it might be worth a sentence
noting that the default deliberately doesn't short-circuit on a no-op
transformer. Overrides that cache are free to skip it, but the contract
currently leaves that unsaid.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java:
##########
@@ -188,6 +189,23 @@ boolean isNull(int docId)
Object getValue(int docId)
throws IOException;
+ /// Applies the supplied deterministic transformer to the value at the given
document id. By default, returns
+ /// `transformer.transform(getValue(docId))`, passing null values to the
transformer as well.
+ ///
+ /// Readers may override this method to avoid materializing repeated raw
values or to reuse immutable conversion
+ /// results. Overrides must preserve the conversion result, including nulls
and conversion failures; callers must not
+ /// depend on the converter's invocation count. 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
Review Comment:
The doc calls the transformer `deterministic`, but nothing enforces it and
the default works either way. Consider phrasing it as a requirement on
callers/overrides so it reads as a contract rather than an assumption.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java:
##########
@@ -188,6 +189,23 @@ boolean isNull(int docId)
Object getValue(int docId)
throws IOException;
+ /// Applies the supplied deterministic transformer to the value at the given
document id. By default, returns
+ /// `transformer.transform(getValue(docId))`, passing null values to the
transformer as well.
+ ///
+ /// Readers may override this method to avoid materializing repeated raw
values or to reuse immutable conversion
+ /// results. Overrides must preserve the conversion result, including nulls
and conversion failures; callers must not
+ /// depend on the converter's invocation count. 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)
Review Comment:
Since `ColumnTransformer` exposes `isNoOp()`, it might be worth a sentence
noting that the default deliberately doesn't short-circuit on a no-op
transformer. Overrides that cache are free to skip it, but the contract
currently leaves that unsaid.
--
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]
