Copilot commented on code in PR #18435:
URL: https://github.com/apache/pinot/pull/18435#discussion_r3198845608
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java:
##########
@@ -70,22 +71,20 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
}
TransformFunction firstArgument = arguments.get(0);
- if (firstArgument instanceof IdentifierTransformFunction) {
- String columnName = ((IdentifierTransformFunction)
firstArgument).getColumnName();
- _jsonIndexReader =
columnContextMap.get(columnName).getDataSource().getJsonIndex();
- if (_jsonIndexReader == null) { //TODO: rework
- Optional<IndexType<?, ?, ?>> compositeIndex =
- IndexService.getInstance().getOptional("composite_json_index");
+ DataSource dataSource = firstArgument.getDataSource();
+ if (dataSource != null) {
+ _jsonIndexReader = dataSource.getJsonIndex();
+ if (_jsonIndexReader == null) { // TODO: rework
+ Optional<IndexType<?, ?, ?>> compositeIndex =
IndexService.getInstance().getOptional("composite_json_index");
if (compositeIndex.isPresent()) {
- _jsonIndexReader = (JsonIndexReader) columnContextMap.get(columnName)
- .getDataSource().getIndex(compositeIndex.get());
+ _jsonIndexReader = (JsonIndexReader)
dataSource.getIndex(compositeIndex.get());
}
}
if (_jsonIndexReader == null) {
throw new IllegalStateException("jsonExtractIndex can only be applied
on a column with JSON index");
}
} else {
- throw new IllegalArgumentException("jsonExtractIndex can only be applied
to a raw column");
+ throw new IllegalArgumentException("jsonExtractIndex can only be applied
to a column with JSON index");
Review Comment:
The `dataSource == null` branch now throws the same high-level message as
the \"no JSON index\" path, but the actionable fix differs (expose/provide a
`DataSource` vs. add/build an index). Consider changing the
`IllegalArgumentException` message to explicitly indicate that the first
argument must be backed by a segment `DataSource` (i.e., an expression that can
expose a source), while keeping the existing message for the case where a
source exists but lacks a JSON index.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/TransformFunction.java:
##########
@@ -79,6 +80,13 @@ default void init(List<TransformFunction> arguments,
Map<String, ColumnContext>
@Nullable
Dictionary getDictionary();
+ /**
+ * Returns the data source backing the transform result when the result maps
directly to a segment data source, or
+ * {@code null} otherwise.
+ */
+ @Nullable
+ DataSource getDataSource();
Review Comment:
Adding a new abstract method to a public interface is a binary-incompatible
change: existing custom/third-party `TransformFunction` implementations
compiled against older versions will fail to load at runtime (e.g.,
`AbstractMethodError`). Consider making this a `default` method returning
`null` to preserve binary compatibility while still allowing implementations to
override and expose a backing `DataSource`.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/TextMatchTransformFunction.java:
##########
@@ -50,15 +50,12 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
super.init(arguments, columnContextMap);
TransformFunction columnArg = arguments.get(0);
- if (!(columnArg instanceof IdentifierTransformFunction &&
columnArg.getResultMetadata().isSingleValue())) {
+ DataSource dataSource = columnArg.getDataSource();
+ if (!columnArg.getResultMetadata().isSingleValue() || dataSource == null) {
throw new IllegalArgumentException(
"The first argument of TEXT_MATCH transform function must be a
single-valued column");
}
Review Comment:
The new guard conflates two different failure modes (multi-value input vs.
missing `DataSource`) but always reports \"must be a single-valued column\".
When `isSingleValue()` is true but `dataSource == null` (e.g., computed
expression or a transform that didn’t expose its source), this message is
inaccurate. Split the checks and provide a specific message for the `dataSource
== null` case (e.g., requiring a segment-backed source with a text index).
--
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]
