fix: Gracefully handle malformed documents in result scanning

[Swiddis]

Description

In general, documents are validated by OpenSearch according to the mapping types at index time. However, this behavior can be bypassed via the index.mapping.ignore_malformed setting, which lets you index arbitrary data regardless of the mapping. This causes PPL to crash any time it encounters these documents.

This PR adds a lot of handling around malformed documents, both at a per-field level (6 individual bad cases involving timestamp, geo, IP, and others) and at an overall level (if a document causes some sort of cascading failure not handled by the prev checks, then we log & skip).

Notes/future work:

• We catch generic Exceptions for field parsing, kinda aggressive but I think for our purposes it's always better to fail one field over failing a doc.
• We don't (yet) indicate in the response body that documents or fields were skipped.
  • We also don't emit a metric (yet) for skipped docs. Willing to take that up in followup on request.
• At time of writing, there's no known way to actually cause document skipping, causing it is always a bug on our part but we still return partial results.
  • Document skips will not be adjusted for limits: a limit-10 query will return 8 results if 2 docs are skipped. This is hard to handle "correctly" in the general case but it's a bug anyway, and we'll see skipped docs in the logs to debug any related issues.

Additionally tested: where malformed_field is null and is not null in queries will work as expected.

Related Issues

Internal ticket.

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

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

⚡ Recommended focus areas for review Limit Mismatch When documents are skipped due to parsing failures, the returned result count can be less than the requested limit. For example, a query with LIMIT 10 may return only 8 results if 2 documents fail to parse. This violates user expectations for limit behavior and makes pagination unreliable. The PR description acknowledges this but does not address it. return Arrays.stream(hits.getHits()) .map( hit -> { try { ImmutableMap.Builder<String, ExprValue> builder = new ImmutableMap.Builder<>(); addParsedHitsToBuilder(builder, hit); addMetaDataFieldsToBuilder(builder, hit); addHighlightsToBuilder(builder, hit); return (ExprValue) ExprTupleValue.fromExprValueMap(builder.build()); } catch (Exception e) { LOG.warn("Failed to parse document {}, skipping", hit.getId(), e); return null; } }) .filter(Objects::nonNull) .iterator(); Silent Null Conversion Catching all Exceptions and returning ExprNullValue.of() silently converts any parsing error (including unexpected runtime exceptions like NullPointerException or OutOfMemoryError) into a null value. This masks bugs in the parsing logic itself. If a field parser has a defect that throws an unexpected exception, it will be hidden rather than surfaced. Consider catching only expected parsing exceptions (e.g., IllegalArgumentException, DateTimeParseException, OpenSearchParseException) to allow genuine bugs to propagate. try { return typeActionMap.get(type).apply(content, type); } catch (Exception e) { return ExprNullValue.of(); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add logging for parsing failures Catching generic Exception silently suppresses all errors without logging, making debugging difficult. Log the exception with context about the field and content being parsed before returning null. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java [212-216] try { return parseGeoPoint(content, supportArrays); } catch (Exception e) { + LOG.warn("Failed to parse geo_point field '{}' with content: {}", field, content, e); return ExprNullValue.of(); } Suggestion importance[1-10]: 7 __ Why: Adding logging for parsing failures would improve debugging capabilities. However, the suggestion assumes a LOG variable exists in this class, which is not shown in the diff. The suggestion is valid if logging infrastructure is available.	Medium
	Add logging for type conversion failures Catching generic Exception silently suppresses all errors without logging. Add logging with field and content context to aid debugging when type conversion fails. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java [227-231] try { return typeActionMap.get(type).apply(content, type); } catch (Exception e) { + LOG.warn("Failed to parse field '{}' of type {} with content: {}", field, type, content, e); return ExprNullValue.of(); } Suggestion importance[1-10]: 7 __ Why: Similar to the first suggestion, adding logging would help with debugging type conversion failures. The suggestion assumes a LOG variable exists in this class, which is not evident from the diff provided.	Medium
	Consider using Optional for safer handling Returning null from the stream and filtering it out may cause issues if the stream processing expects non-null values. Consider using Optional or a sentinel value to make the intent clearer and safer. opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java [169-172] +} catch (Exception e) { + LOG.warn("Failed to parse document {}, skipping", hit.getId(), e); + return null; +} - Suggestion importance[1-10]: 3 __ Why: The suggestion doesn't provide actual improvement in the improved_code section - it's identical to existing_code. The current implementation with filter(Objects::nonNull) at line 174 already handles null values safely, making this suggestion less impactful.	Low