Fix/array type ip binary by vinaykpud · Pull Request #5516 · opensearch-project/sql

vinaykpud · 2026-06-04T19:04:14Z

Description

Companion fix for the LIST nullability change in opensearch-project/OpenSearch#21997. After that fix, stats list(<field>) returns HTTP 200 for 10 of 12 element types — but list(ip_value) and list(binary_value) still fail with HTTP 400 ExpressionEvaluationException: unsupported object class [B. This PR fixes those two.

Root cause

Two layers, each correct in isolation:

PPLBuiltinOperators.LIST was registered with return type PPLReturnTypes.STRING_ARRAY, which always returns ARRAY<VARCHAR> regardless of input element type — type information is lost.
The UDT-aware response-boundary dispatch from Added UDT for IP and Binary support #5463 (AnalyticsExecutionEngine.toExprValue) checks type instanceof IpType / BinaryType per cell. With the column type collapsed to ARRAY<VARCHAR> it never matches, so per-element byte[] values fall through to ExprValueUtils.fromObjectValue, which has no byte[] case and throws.

Fix

Two small changes:

PPLBuiltinOperators.LIST: switch return-type inference from STRING_ARRAY to ARG0_ARRAY, so the column type at the response boundary becomes ARRAY<IpType> / ARRAY<BinaryType> (matching the existing TAKE registration).
AnalyticsExecutionEngine.toExprValue: when the column is ARRAY<IpType> / ARRAY<BinaryType>, recurse element-wise and apply the same canonical-IP-string / Base64 conversion already used for scalar IpType / BinaryType cells. Extracted the two byte[] converters into helpers (ipBytesToExprValue, binaryBytesToExprValue) so the scalar and array paths share them.

Testing

Query	Before	After
`stats list(ip_value)`	HTTP 400 `unsupported object class [B`	`[["127.0.0.1"]]`
`stats list(binary_value)`	HTTP 400 `unsupported object class [B`	`[["U29tZSBiaW5hcnkgYmxvYg=="]]`
`stats list(<other 10 types>)`	HTTP 200	HTTP 200 (no regression)
`\| fields ip_value, binary_value`	HTTP 200 (#5463 path)	HTTP 200 (no regression)

Out of scope

VALUES aggregate registration still uses STRING_ARRAY. Same fix would apply if anyone exercises values(ip_value) / values(binary_value); flagging for follow-up.
ListAggFunction.java still does String.valueOf(byte[]) — latent on the V3 (non-analytics-engine) path. Analytics path uses DataFusion's LOCAL_ARRAY_AGG_OP instead, so it doesn't fire here.

github-actions · 2026-06-04T19:05:11Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 352e53c.

Path	Line	Severity	Description
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java	111	critical	The entire body of isAnalyticsIndex() has been commented out and replaced with an unconditional 'return true'. This bypasses all routing guards — cluster-level setting checks, per-index pluggable-dataformat checks, and the null/empty query guard — forcing every query to be treated as an analytics query and routed through the Calcite execution engine regardless of configuration. The original logic is preserved verbatim as comments, strongly suggesting deliberate intent to leave a reversible backdoor. This change is entirely unrelated to the stated PR purpose of fixing list() aggregate type handling, making it highly anomalous.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 1 | High: 0 | Medium: 0 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-06-04T19:08:26Z

PR Reviewer Guide 🔍

(Review updated until commit `fec82ff`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Possible Issue When processing `ARRAY<IpType>` or `ARRAY<BinaryType>`, if a list element is neither null nor `byte[]`, it falls through to `ExprValueUtils.fromObjectValue(elem)`. If `fromObjectValue` cannot handle that element type, it will throw an exception. This can occur if the upstream data source returns unexpected element types for IP or binary arrays. The code assumes all non-null elements are `byte[]`, but does not enforce or validate this assumption. if (value instanceof List<?> list) { RelDataType component = type.getComponentType(); if (component instanceof IpType) { List<ExprValue> elems = new ArrayList<>(list.size()); for (Object elem : list) { if (elem == null) { elems.add(ExprValueUtils.nullValue()); } else if (elem instanceof byte[] eb) { elems.add(ipBytesToExprValue(eb)); } else { elems.add(ExprValueUtils.fromObjectValue(elem)); } } return new ExprCollectionValue(elems); } else if (component instanceof BinaryType) { List<ExprValue> elems = new ArrayList<>(list.size()); for (Object elem : list) { if (elem == null) { elems.add(ExprValueUtils.nullValue()); } else if (elem instanceof byte[] eb) { elems.add(binaryBytesToExprValue(eb)); } else { elems.add(ExprValueUtils.fromObjectValue(elem)); } } return new ExprCollectionValue(elems);

github-actions · 2026-06-04T19:08:49Z

PR Code Suggestions ✨

Latest suggestions up to fec82ff

Explore these optional code suggestions:

Category Suggestion Impact

General

Extract duplicated list conversion logic

The code duplicates the list conversion logic for IpType and BinaryType. Extract the
common iteration pattern into a helper method that accepts a conversion function,
reducing code duplication and improving maintainability.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261]

 if (value instanceof List<?> list) {
   RelDataType component = type.getComponentType();
   if (component instanceof IpType) {
-    List<ExprValue> elems = new ArrayList<>(list.size());
-    for (Object elem : list) {
-      if (elem == null) {
-        elems.add(ExprValueUtils.nullValue());
-      } else if (elem instanceof byte[] eb) {
-        elems.add(ipBytesToExprValue(eb));
-      } else {
-        elems.add(ExprValueUtils.fromObjectValue(elem));
-      }
-    }
-    return new ExprCollectionValue(elems);
+    return convertListToExprCollection(list, this::ipBytesToExprValue);
   } else if (component instanceof BinaryType) {
-    List<ExprValue> elems = new ArrayList<>(list.size());
-    for (Object elem : list) {
-      if (elem == null) {
-        elems.add(ExprValueUtils.nullValue());
-      } else if (elem instanceof byte[] eb) {
-        elems.add(binaryBytesToExprValue(eb));
-      } else {
-        elems.add(ExprValueUtils.fromObjectValue(elem));
-      }
-    }
-    return new ExprCollectionValue(elems);
+    return convertListToExprCollection(list, this::binaryBytesToExprValue);
   }
 }
 
+private static ExprCollectionValue convertListToExprCollection(
+    List<?> list, Function<byte[], ExprValue> converter) {
+  List<ExprValue> elems = new ArrayList<>(list.size());
+  for (Object elem : list) {
+    if (elem == null) {
+      elems.add(ExprValueUtils.nullValue());
+    } else if (elem instanceof byte[] eb) {
+      elems.add(converter.apply(eb));
+    } else {
+      elems.add(ExprValueUtils.fromObjectValue(elem));
+    }
+  }
+  return new ExprCollectionValue(elems);
+}
+

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies code duplication between IpType and BinaryType list conversion logic. Extracting this into a helper method would improve maintainability and reduce redundancy. However, the impact is moderate as the current code is functional and the duplication is limited to two cases.

Medium

Add null check for component type

Add a null check for type.getComponentType() before using it. If the type is an
array but getComponentType() returns null, the code could fail unexpectedly when
processing malformed schema definitions.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261]

 if (value instanceof List<?> list) {
   RelDataType component = type.getComponentType();
+  if (component == null) {
+    return ExprValueUtils.fromObjectValue(value);
+  }
   if (component instanceof IpType) {
     ...
   } else if (component instanceof BinaryType) {
     ...
   }
 }

Suggestion importance[1-10]: 6

__

Why: Adding a null check for type.getComponentType() is a reasonable defensive programming practice that could prevent potential NPEs with malformed schemas. However, this is a precautionary measure rather than fixing an observed bug, and the likelihood of encountering a null component type in practice may be low given the context.

Low

Previous suggestions

Suggestions up to commit b4a67c5

Category Suggestion Impact

General

Eliminate duplicate list conversion logic

The duplicate list-processing logic for IpType and BinaryType should be refactored
into a single helper method to reduce code duplication. This improves
maintainability and reduces the risk of inconsistent behavior between the two
branches.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261]

 if (value instanceof List<?> list) {
   RelDataType component = type.getComponentType();
   if (component instanceof IpType) {
-    List<ExprValue> elems = new ArrayList<>(list.size());
-    for (Object elem : list) {
-      if (elem == null) {
-        elems.add(ExprValueUtils.nullValue());
-      } else if (elem instanceof byte[] eb) {
-        elems.add(ipBytesToExprValue(eb));
-      } else {
-        elems.add(ExprValueUtils.fromObjectValue(elem));
-      }
-    }
-    return new ExprCollectionValue(elems);
+    return convertListToExprCollection(list, AnalyticsExecutionEngine::ipBytesToExprValue);
   } else if (component instanceof BinaryType) {
-    List<ExprValue> elems = new ArrayList<>(list.size());
-    for (Object elem : list) {
-      if (elem == null) {
-        elems.add(ExprValueUtils.nullValue());
-      } else if (elem instanceof byte[] eb) {
-        elems.add(binaryBytesToExprValue(eb));
-      } else {
-        elems.add(ExprValueUtils.fromObjectValue(elem));
-      }
-    }
-    return new ExprCollectionValue(elems);
+    return convertListToExprCollection(list, AnalyticsExecutionEngine::binaryBytesToExprValue);
   }
 }
 
+private static ExprCollectionValue convertListToExprCollection(
+    List<?> list, Function<byte[], ExprValue> converter) {
+  List<ExprValue> elems = new ArrayList<>(list.size());
+  for (Object elem : list) {
+    if (elem == null) {
+      elems.add(ExprValueUtils.nullValue());
+    } else if (elem instanceof byte[] eb) {
+      elems.add(converter.apply(eb));
+    } else {
+      elems.add(ExprValueUtils.fromObjectValue(elem));
+    }
+  }
+  return new ExprCollectionValue(elems);
+}
+

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies code duplication between IpType and BinaryType list processing. Refactoring into a helper method would improve maintainability and reduce duplication, though the current code is functional and the improvement is primarily stylistic.

Medium

Handle null component type safely

Add a null check for type.getComponentType() before using it. If the type is an
array but getComponentType() returns null, the code will proceed to fromObjectValue
which may not handle the list correctly, potentially causing unexpected behavior.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261]

 if (value instanceof List<?> list) {
   RelDataType component = type.getComponentType();
+  if (component == null) {
+    return ExprValueUtils.fromObjectValue(value);
+  }
   if (component instanceof IpType) {
     ...
   } else if (component instanceof BinaryType) {
     ...
   }
 }

Suggestion importance[1-10]: 6

__

Why: Adding a null check for getComponentType() is a defensive programming practice that could prevent potential issues. However, the current code already falls through to fromObjectValue if neither IpType nor BinaryType match, which may handle this case adequately.

Low

Suggestions up to commit b491034

Category Suggestion Impact

Possible issue

Add null check for component type

Add a null check for type.getComponentType() before using it. If the type is not an
array type, getComponentType() may return null, causing a NullPointerException when
checking instanceof IpType or instanceof BinaryType.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261]

 if (value instanceof List<?> list) {
   RelDataType component = type.getComponentType();
+  if (component == null) {
+    return ExprValueUtils.fromObjectValue(value);
+  }
   if (component instanceof IpType) {
     List<ExprValue> elems = new ArrayList<>(list.size());
     for (Object elem : list) {
       if (elem == null) {
         elems.add(ExprValueUtils.nullValue());
       } else if (elem instanceof byte[] eb) {
         elems.add(ipBytesToExprValue(eb));
       } else {
         elems.add(ExprValueUtils.fromObjectValue(elem));
       }
     }
     return new ExprCollectionValue(elems);
   } else if (component instanceof BinaryType) {
     ...
   }
 }

Suggestion importance[1-10]: 7

__

Why: Valid defensive programming suggestion. If getComponentType() returns null for non-array types, the instanceof checks could fail. However, the context suggests this code path is only reached for array types, making this a precautionary rather than critical fix.

Medium

General

Extract duplicate array conversion logic

The duplicate code blocks for IpType and BinaryType array handling differ only in
the conversion function called. Extract this logic into a helper method that accepts
a conversion function to eliminate code duplication and improve maintainability.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [238-260]

-List<ExprValue> elems = new ArrayList<>(list.size());
-for (Object elem : list) {
-  if (elem == null) {
-    elems.add(ExprValueUtils.nullValue());
-  } else if (elem instanceof byte[] eb) {
-    elems.add(ipBytesToExprValue(eb));
-  } else {
-    elems.add(ExprValueUtils.fromObjectValue(elem));
+return convertListToExprCollection(list, this::ipBytesToExprValue);
+
+// Helper method:
+private static ExprCollectionValue convertListToExprCollection(
+    List<?> list, Function<byte[], ExprValue> byteConverter) {
+  List<ExprValue> elems = new ArrayList<>(list.size());
+  for (Object elem : list) {
+    if (elem == null) {
+      elems.add(ExprValueUtils.nullValue());
+    } else if (elem instanceof byte[] eb) {
+      elems.add(byteConverter.apply(eb));
+    } else {
+      elems.add(ExprValueUtils.fromObjectValue(elem));
+    }
   }
+  return new ExprCollectionValue(elems);
 }

Suggestion importance[1-10]: 6

__

Why: Good refactoring suggestion to reduce code duplication between IpType and BinaryType array handling. The improved code correctly uses a function parameter to handle the conversion difference, improving maintainability.

Low

Suggestions up to commit b491034

Category	Suggestion	Impact
Possible issue	Add null check for component type Add a null check for `type.getComponentType()` before using it. If the type is a list but doesn't have a component type, this could result in a `NullPointerException` when checking `component instanceof IpType`. core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261] if (value instanceof List<?> list) { RelDataType component = type.getComponentType(); + if (component == null) { + return ExprValueUtils.fromObjectValue(value); + } if (component instanceof IpType) { List<ExprValue> elems = new ArrayList<>(list.size()); for (Object elem : list) { if (elem == null) { elems.add(ExprValueUtils.nullValue()); } else if (elem instanceof byte[] eb) { elems.add(ipBytesToExprValue(eb)); } else { elems.add(ExprValueUtils.fromObjectValue(elem)); } } return new ExprCollectionValue(elems); } else if (component instanceof BinaryType) { ... } } Suggestion importance[1-10]: 7 __ Why: Adding a null check for `component` before using it prevents potential `NullPointerException` when `type.getComponentType()` returns null. This is a defensive programming practice that improves robustness, though the likelihood of encountering this scenario depends on the upstream type system guarantees.	Medium
General	Extract duplicate list conversion logic The duplicate code blocks for `IpType` and `BinaryType` array handling differ only in the conversion function called. Extract this logic into a helper method that accepts a conversion function to eliminate code duplication and improve maintainability. core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [238-260] -List<ExprValue> elems = new ArrayList<>(list.size()); -for (Object elem : list) { - if (elem == null) { - elems.add(ExprValueUtils.nullValue()); - } else if (elem instanceof byte[] eb) { - elems.add(ipBytesToExprValue(eb)); - } else { - elems.add(ExprValueUtils.fromObjectValue(elem)); +return convertListToExprCollection(list, this::ipBytesToExprValue); + +// Helper method: +private static ExprCollectionValue convertListToExprCollection( + List<?> list, Function<byte[], ExprValue> byteArrayConverter) { + List<ExprValue> elems = new ArrayList<>(list.size()); + for (Object elem : list) { + if (elem == null) { + elems.add(ExprValueUtils.nullValue()); + } else if (elem instanceof byte[] eb) { + elems.add(byteArrayConverter.apply(eb)); + } else { + elems.add(ExprValueUtils.fromObjectValue(elem)); + } } + return new ExprCollectionValue(elems); } -return new ExprCollectionValue(elems); Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies code duplication between `IpType` and `BinaryType` list handling blocks. Extracting this into a helper method would improve maintainability and reduce code duplication, though the impact is moderate since the duplicated code is relatively small and localized.	Low

Suggestions up to commit 2734707

Category Suggestion Impact

Possible issue

Add null check for component type

Add a null check for component before using instanceof to prevent potential
NullPointerException. The getComponentType() method may return null for non-array
types, which would cause the instanceof checks to fail unexpectedly.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261]

 if (value instanceof List<?> list) {
   RelDataType component = type.getComponentType();
-  if (component instanceof IpType) {
-    List<ExprValue> elems = new ArrayList<>(list.size());
-    for (Object elem : list) {
-      if (elem == null) {
-        elems.add(ExprValueUtils.nullValue());
-      } else if (elem instanceof byte[] eb) {
-        elems.add(ipBytesToExprValue(eb));
-      } else {
-        elems.add(ExprValueUtils.fromObjectValue(elem));
+  if (component != null) {
+    if (component instanceof IpType) {
+      List<ExprValue> elems = new ArrayList<>(list.size());
+      for (Object elem : list) {
+        if (elem == null) {
+          elems.add(ExprValueUtils.nullValue());
+        } else if (elem instanceof byte[] eb) {
+          elems.add(ipBytesToExprValue(eb));
+        } else {
+          elems.add(ExprValueUtils.fromObjectValue(elem));
+        }
       }
+      return new ExprCollectionValue(elems);
+    } else if (component instanceof BinaryType) {
+      ...
     }
-    return new ExprCollectionValue(elems);
-  } else if (component instanceof BinaryType) {
-    ...
   }
 }

Suggestion importance[1-10]: 7

__

Why: Adding a null check for component before using instanceof is a valid defensive programming practice. The getComponentType() method could return null for non-array types, and while the code may work without it in the current context, the check prevents potential NullPointerException and makes the code more robust.

Medium

General

Extract duplicated list conversion logic

Extract the duplicated list conversion logic for IpType and BinaryType into a
separate helper method. This reduces code duplication and improves maintainability
since both branches have identical structure with only the conversion function
differing.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [238-260]

-List<ExprValue> elems = new ArrayList<>(list.size());
-for (Object elem : list) {
-  if (elem == null) {
-    elems.add(ExprValueUtils.nullValue());
-  } else if (elem instanceof byte[] eb) {
-    elems.add(ipBytesToExprValue(eb));
-  } else {
-    elems.add(ExprValueUtils.fromObjectValue(elem));
+return convertListToExprCollection(list, this::ipBytesToExprValue);
+
+// Helper method:
+private static ExprCollectionValue convertListToExprCollection(
+    List<?> list, Function<byte[], ExprValue> byteArrayConverter) {
+  List<ExprValue> elems = new ArrayList<>(list.size());
+  for (Object elem : list) {
+    if (elem == null) {
+      elems.add(ExprValueUtils.nullValue());
+    } else if (elem instanceof byte[] eb) {
+      elems.add(byteArrayConverter.apply(eb));
+    } else {
+      elems.add(ExprValueUtils.fromObjectValue(elem));
+    }
   }
+  return new ExprCollectionValue(elems);
 }
-return new ExprCollectionValue(elems);

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies code duplication between the IpType and BinaryType list conversion branches. Extracting this into a helper method would improve maintainability and reduce duplication, though the impact is moderate since the duplicated code is localized and relatively simple.

Low

Suggestions up to commit 69d24da

Category Suggestion Impact

Possible issue

Add null check for component type

Add null check for component before using instanceof checks. If
type.getComponentType() returns null for non-array types, the subsequent instanceof
checks could behave unexpectedly or mask errors.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [235-261]

 if (value instanceof List<?> list) {
   RelDataType component = type.getComponentType();
-  if (component instanceof IpType) {
-    List<ExprValue> elems = new ArrayList<>(list.size());
-    for (Object elem : list) {
-      if (elem == null) {
-        elems.add(ExprValueUtils.nullValue());
-      } else if (elem instanceof byte[] eb) {
-        elems.add(ipBytesToExprValue(eb));
-      } else {
-        elems.add(ExprValueUtils.fromObjectValue(elem));
+  if (component != null) {
+    if (component instanceof IpType) {
+      List<ExprValue> elems = new ArrayList<>(list.size());
+      for (Object elem : list) {
+        if (elem == null) {
+          elems.add(ExprValueUtils.nullValue());
+        } else if (elem instanceof byte[] eb) {
+          elems.add(ipBytesToExprValue(eb));
+        } else {
+          elems.add(ExprValueUtils.fromObjectValue(elem));
+        }
       }
+      return new ExprCollectionValue(elems);
+    } else if (component instanceof BinaryType) {
+      List<ExprValue> elems = new ArrayList<>(list.size());
+      for (Object elem : list) {
+        if (elem == null) {
+          elems.add(ExprValueUtils.nullValue());
+        } else if (elem instanceof byte[] eb) {
+          elems.add(binaryBytesToExprValue(eb));
+        } else {
+          elems.add(ExprValueUtils.fromObjectValue(elem));
+        }
+      }
+      return new ExprCollectionValue(elems);
     }
-    return new ExprCollectionValue(elems);
-  } else if (component instanceof BinaryType) {
-    List<ExprValue> elems = new ArrayList<>(list.size());
-    for (Object elem : list) {
-      if (elem == null) {
-        elems.add(ExprValueUtils.nullValue());
-      } else if (elem instanceof byte[] eb) {
-        elems.add(binaryBytesToExprValue(eb));
-      } else {
-        elems.add(ExprValueUtils.fromObjectValue(elem));
-      }
-    }
-    return new ExprCollectionValue(elems);
   }
 }

Suggestion importance[1-10]: 7

__

Why: Adding a null check for component before instanceof checks is a valid defensive programming practice. If getComponentType() returns null for non-array types, the current code would skip the special handling and fall through to fromObjectValue, which is reasonable but could mask issues. The suggestion improves robustness.

Medium

github-actions · 2026-06-04T19:09:48Z

Persistent review updated to latest commit 69d24da

github-actions · 2026-06-04T19:16:54Z

Persistent review updated to latest commit 2734707

ahkcs · 2026-06-04T20:34:11Z

Please take a look at the CI failures

github-actions · 2026-06-05T00:37:23Z

Persistent review updated to latest commit b491034

github-actions · 2026-06-05T05:24:12Z

Persistent review updated to latest commit b491034

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

github-actions · 2026-06-05T20:12:22Z

Persistent review updated to latest commit b4a67c5

V3 LIST accumulator was stringifying every element via String.valueOf, which conflicted with the ARG0_ARRAY return-type declaration on PPLBuiltinOperators.LIST and threw ClassCastException at runtime (e.g. (Boolean) "true"). Drop the String.valueOf and accumulate raw Object values; update CalciteMultiValueStatsIT numeric/boolean assertions to match the unquoted JSON output. Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

github-actions · 2026-06-05T20:13:53Z

Persistent review updated to latest commit fec82ff

penghuo · 2026-06-05T20:21:57Z

                TEST_INDEX_DATATYPE_NONNUMERIC));
    verifySchema(response, schema("bool_list", "array"));
-    verifyDataRows(response, rows(List.of("true")));
+    verifyDataRows(response, rows(List.of(true)));


@ps48 Do u remember any reasons we enforce return ARRAY(STRING) in PPL?

This was based on industry standards and workings of multivalue stats commands.

Can you provide example on this why the return value is always kept string?

here's the RFC and the decisions we made through the initial implementation #4026

Thanks @penghuo @ps48 , I fixed it by rewriting the plan which will keep the contract same.
opensearch-project/OpenSearch#21997

vinaykpud requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners June 4, 2026 19:04

vinaykpud force-pushed the fix/array_type_ip_binary branch from b1f89a9 to b567be6 Compare June 4, 2026 19:07

vinaykpud force-pushed the fix/array_type_ip_binary branch from b567be6 to 69d24da Compare June 4, 2026 19:08

vinaykpud force-pushed the fix/array_type_ip_binary branch from 69d24da to 2734707 Compare June 4, 2026 19:15

vinaykpud added enhancement New feature or request PPL Piped processing language labels Jun 4, 2026

vinaykpud force-pushed the fix/array_type_ip_binary branch from 2734707 to dad3901 Compare June 4, 2026 21:11

vinaykpud closed this Jun 4, 2026

vinaykpud reopened this Jun 4, 2026

vinaykpud force-pushed the fix/array_type_ip_binary branch from dad3901 to b491034 Compare June 5, 2026 00:36

vinaykpud closed this Jun 5, 2026

vinaykpud reopened this Jun 5, 2026

penghuo reviewed Jun 5, 2026

View reviewed changes

Comment thread core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java

vinaykpud force-pushed the fix/array_type_ip_binary branch from b491034 to 352e53c Compare June 5, 2026 20:10

vinaykpud added 2 commits June 5, 2026 20:10

Preserve UDT element type for list(ip|binary) aggregates

d356557

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

spotless fix

08e68e4

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

vinaykpud force-pushed the fix/array_type_ip_binary branch from 352e53c to b4a67c5 Compare June 5, 2026 20:11

vinaykpud force-pushed the fix/array_type_ip_binary branch from b4a67c5 to fec82ff Compare June 5, 2026 20:12

penghuo reviewed Jun 5, 2026

View reviewed changes

vinaykpud closed this Jun 5, 2026

Conversation

vinaykpud commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Root cause

Fix

Testing

Out of scope

Uh oh!

github-actions Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit fec82ff)

Uh oh!

github-actions Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

ahkcs commented Jun 4, 2026

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

penghuo Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

ps48 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

vinaykpud Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ps48 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

vinaykpud Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vinaykpud commented Jun 4, 2026 •

edited

Loading

github-actions Bot commented Jun 4, 2026 •

edited

Loading

github-actions Bot commented Jun 4, 2026 •

edited

Loading

(Review updated until commit `fec82ff`)

github-actions Bot commented Jun 4, 2026 •

edited

Loading

vinaykpud Jun 5, 2026 •

edited

Loading

vinaykpud Jun 5, 2026 •

edited

Loading