Skip to content

Fix PPL foreach JSON array type coercion - #5637

Merged
penghuo merged 1 commit into
opensearch-project:mainfrom
songkant-aws:feature/foreach-type-coercion-followup
Jul 20, 2026
Merged

Fix PPL foreach JSON array type coercion#5637
penghuo merged 1 commit into
opensearch-project:mainfrom
songkant-aws:feature/foreach-type-coercion-followup

Conversation

@songkant-aws

Copy link
Copy Markdown
Collaborator

Description

This is a follow-up to #5613 for JSON arrays stored in text fields.

For field-backed JSON, element values are not visible while the query is being planned. Numeric use of <<ITEM>> therefore selects a concrete numeric element type from the expression context, but ForeachJsonArrayFunctionImpl previously assumed every parsed value was already a Number. A JSON string in that path caused a ClassCastException and an HTTP 500 response.

This change:

  • converts numeric JSON elements with Calcite's runtime conversion helpers, including numeric strings;
  • returns null when a value cannot be converted to the inferred concrete type instead of throwing;
  • keeps ARRAY<DOUBLE> and ARRAY<VARCHAR> return types concrete rather than introducing ANY or runtime-dependent operator coercion;
  • adds unit and indexed-field integration coverage for failed numeric conversion;
  • marks foreach as experimental and removes product-comparison wording from plugin documentation and test comments.

User impact

Field-backed JSON arrays used by numeric foreach expressions no longer fail the request when an element is not numeric. Valid numeric values and numeric strings continue to use numeric evaluation, while incompatible values produce null.

Validation

./gradlew :core:test --tests org.opensearch.sql.expression.function.CollectionUDF.ForeachFunctionImplTest :ppl:test --tests org.opensearch.sql.ppl.calcite.CalcitePPLForeachTest

The change also adds the affected ForeachFieldJsonIT coverage to the no-pushdown integration suite.

Related review: #5613 (comment)

Signed-off-by: Songkan Tang <songkant@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Catch specific conversion exceptions

The catch block silently swallows all RuntimeException types, which may hide
unexpected errors beyond type coercion failures. Consider catching more specific
exceptions like NumberFormatException or logging the exception before returning null
to aid debugging.

core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/ForeachJsonArrayFunctionImpl.java [101-113]

 try {
   return switch (elementType) {
     case DOUBLE -> SqlFunctions.toDouble(value);
     case VARCHAR ->
         value instanceof List<?> || value instanceof java.util.Map<?, ?>
             ? gson.toJson(value)
             : String.valueOf(value);
     case DECIMAL -> SqlFunctions.toBigDecimal(value);
     default -> value;
   };
-} catch (RuntimeException e) {
+} catch (NumberFormatException | ClassCastException e) {
   return null;
 }
Suggestion importance[1-10]: 3

__

Why: While catching specific exceptions is generally better practice, the current implementation intentionally matches Calcite's SAFE_CAST semantics (as noted in the comment), which returns null for any conversion failure. The suggestion to catch only NumberFormatException and ClassCastException may not cover all conversion failures from SqlFunctions.toDouble() and SqlFunctions.toBigDecimal(), potentially breaking the intended safe cast behavior.

Low

@songkant-aws

Copy link
Copy Markdown
Collaborator Author

@penghuo As per merged PR disucssion: #5613 (comment), I've changed document phrase and coercion behavior from parsing unknown dynamic type directly from index storage. For now, I haven't find a good solution to immediately solve that.

Additionally, none of the user’s example queries use json_str mode. All of them involve multifield mode, where the type is known in advance and the field value can be normalized accordingly. Therefore, I decided to fall back to null when the type is ambiguous in this case.

@penghuo
penghuo merged commit 29c7ecb into opensearch-project:main Jul 20, 2026
43 of 44 checks passed
asifabashar pushed a commit to asifabashar/sql that referenced this pull request Jul 21, 2026
Signed-off-by: Songkan Tang <songkant@amazon.com>
@songkant-aws
songkant-aws deleted the feature/foreach-type-coercion-followup branch July 21, 2026 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants