Copilot commented on code in PR #16306:
URL: https://github.com/apache/pinot/pull/16306#discussion_r2212266172
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractKeyTransformFunction.java:
##########
@@ -90,8 +102,24 @@ public String[][] transformToStringValuesMV(ValueBlock
valueBlock) {
initStringValuesMV(length);
String[] jsonStrings =
_jsonFieldTransformFunction.transformToStringValuesSV(valueBlock);
for (int i = 0; i < length; i++) {
- List<String> values =
JSON_PARSER_CONTEXT.parse(jsonStrings[i]).read(_jsonPath);
- _stringValuesMV[i] = values.toArray(new String[0]);
+ // Call the appropriate JsonFunctions method based on available
parameters
+ List values;
Review Comment:
[nitpick] Use a parameterized type (`List<String>`) instead of a raw `List`
for `values` to improve type safety.
```suggestion
List<String> values;
```
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/ParserUtils.java:
##########
@@ -77,15 +77,25 @@ private static void
validateJsonExtractScalarFunction(List<Expression> operands)
}
private static void validateJsonExtractKeyFunction(List<Expression>
operands) {
- // Check that there are exactly 2 arguments
- if (operands.size() != 2) {
+ // Check that there are 2, 3, or 4 arguments
+ if (operands.size() < 2 || operands.size() > 3) {
throw new SqlCompilationException(
- "Expect 2 arguments are required for transform function:
jsonExtractKey(jsonFieldName, 'jsonPath')");
+ "2, or 3 arguments are required for transform function: "
Review Comment:
[nitpick] The error message reads "2, or 3 arguments are required"—remove
the comma for clarity: "2 or 3 arguments are required".
```suggestion
"2 or 3 arguments are required for transform function: "
```
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractKeyTransformFunction.java:
##########
@@ -63,10 +61,11 @@ public String getName() {
@Override
public void init(List<TransformFunction> arguments, Map<String,
ColumnContext> columnContextMap) {
super.init(arguments, columnContextMap);
- // Check that there are exactly 2 arguments
- if (arguments.size() != 2) {
+ // Check that there are 2, 3, or 4 arguments
+ if (arguments.size() < 2 || arguments.size() > 4) {
Review Comment:
The arity check allows up to 4 arguments but only 2 or 3 are supported.
Change the upper bound to > 3 to reject four or more arguments.
```suggestion
// Check that there are 2 or 3 arguments
if (arguments.size() < 2 || arguments.size() > 3) {
```
--
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]
