Add include_metadata request parameter for PPL queries #5235

[ishag4]

Description

Add a request-level parameter include_metadata to the PPL query API:

POST /_plugins/_ppl?include_metadata=true
{
"query": "source=logs | where level='ERROR' | fields * | head 10"
}
Result: All regular fields PLUS metadata fields (_id, _index, _score, etc.)

Related Issues

Resolves #5235

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 🔍

(Review updated until commit 7194590)

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 Logic Inversion The logic for handling AllFieldsExcludeMeta vs AllFields appears inverted. When AllFieldsExcludeMeta is encountered (include_metadata=false), the code forces metadata exclusion. But when AllFields is encountered (include_metadata=true), it calls tryToRemoveMetaFields with excludeByForce=false, which may still remove metadata if isProjectVisited is false. This contradicts the intent that include_metadata=true should include metadata fields. if (allFields instanceof AllFieldsExcludeMeta) { // For AllFieldsExcludeMeta (include_metadata=false), remove nested fields and force exclude // metadata tryToRemoveNestedFields(context); tryToRemoveMetaFields(context, true); // Force exclude metadata fields } else { // For AllFields (include_metadata=true), include metadata fields tryToRemoveNestedFields(context); // Mark as project visited to prevent automatic metadata field removal context.setProjectVisited(true); // Don't force exclude metadata fields - let them remain tryToRemoveMetaFields(context, false); } Conditional Check Issue The condition checks context.isIncludeMetadata() && !excludeByForce to decide whether to return early. However, when excludeByForce is true, the method proceeds to remove metadata fields even if includeMetadata is true. This means forced exclusion overrides the user's explicit request to include metadata, which may not be the intended behavior for all scenarios. if (context.isIncludeMetadata() && !excludeByForce) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 7194590

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Remove redundant metadata filtering call The logic for handling AllFields (include_metadata=true) calls tryToRemoveMetaFields(context, false) which may still remove metadata fields if !context.isProjectVisited() evaluates to true. Since setProjectVisited(true) is called just before, this creates a timing dependency. Consider removing the tryToRemoveMetaFields call entirely for the AllFields case to ensure metadata fields are never removed. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-509] if (allFields instanceof AllFieldsExcludeMeta) { // For AllFieldsExcludeMeta (include_metadata=false), remove nested fields and force exclude // metadata tryToRemoveNestedFields(context); tryToRemoveMetaFields(context, true); // Force exclude metadata fields } else { // For AllFields (include_metadata=true), include metadata fields tryToRemoveNestedFields(context); // Mark as project visited to prevent automatic metadata field removal context.setProjectVisited(true); - // Don't force exclude metadata fields - let them remain - tryToRemoveMetaFields(context, false); + // Don't call tryToRemoveMetaFields at all - metadata fields should remain } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential redundancy where tryToRemoveMetaFields(context, false) is called after setProjectVisited(true). However, examining the implementation shows this call may still serve a purpose for consistency. The suggestion is valid but represents a minor optimization rather than a critical fix.	Medium
	Warn about metadata parameter loss When falling back to the legacy engine, the includeMetadata parameter is not passed through to executeWithLegacy. This means that if a user explicitly requested include_metadata=true and the query falls back to the legacy engine, their preference will be silently ignored. Consider logging a warning about this limitation or passing the parameter if the legacy engine supports it. core/src/main/java/org/opensearch/sql/executor/QueryService.java [216-222] } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { - log.warn("Fallback to V2 query engine since got exception", t); + log.warn("Fallback to V2 query engine since got exception. Note: include_metadata parameter may not be fully supported in legacy engine.", t); // Legacy engine provides basic metadata support, so fallback is acceptable executeWithLegacy(plan, queryType, listener, Optional.of(t)); } else { propagateCalciteError(t, listener); } Suggestion importance[1-10]: 6 __ Why: Valid observation that includeMetadata is not passed to the legacy engine fallback. The suggestion to add a warning is reasonable for user transparency. However, the existing comment already mentions that "Legacy engine provides basic metadata support," so the impact is moderate.	Low
	Clarify force-exclude metadata behavior The condition context.isIncludeMetadata() && !excludeByForce creates a logical issue: when excludeByForce=true, metadata fields will be removed even if includeMetadata=true. This contradicts the user's explicit request to include metadata. Consider whether excludeByForce should truly override the user's includeMetadata preference, or if this represents a bug in subquery/join scenarios. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [656-662] private static void tryToRemoveMetaFields(CalcitePlanContext context, boolean excludeByForce) { - // If include_metadata=true, never remove metadata fields + // If include_metadata=true and not forced by subquery/join context, preserve metadata fields if (context.isIncludeMetadata() && !excludeByForce) { return; } + // Note: excludeByForce=true (from joins/subqueries) will override user's includeMetadata preference + // This may need review if metadata should be preserved in those contexts if (excludeByForce || !context.isProjectVisited()) { Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid design question about whether excludeByForce should override includeMetadata. However, the current behavior appears intentional for subquery/join contexts where metadata exclusion is necessary. The suggestion adds clarifying comments but doesn't identify a clear bug, making it more of a documentation improvement.	Low

---

Previous suggestions

Suggestions up to commit 4f001a6

Category	Suggestion	Impact
General	Remove redundant metadata field filtering call The logic for handling AllFields (include_metadata=true) calls tryToRemoveMetaFields(context, false) which may still remove metadata fields if !context.isProjectVisited() evaluates to true. Since setProjectVisited(true) is called just before, this creates a timing dependency. Consider removing the tryToRemoveMetaFields call entirely for the AllFields case to ensure metadata fields are never removed. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-509] if (allFields instanceof AllFieldsExcludeMeta) { // For AllFieldsExcludeMeta (include_metadata=false), remove nested fields and force exclude // metadata tryToRemoveNestedFields(context); tryToRemoveMetaFields(context, true); // Force exclude metadata fields } else { // For AllFields (include_metadata=true), include metadata fields tryToRemoveNestedFields(context); // Mark as project visited to prevent automatic metadata field removal context.setProjectVisited(true); - // Don't force exclude metadata fields - let them remain - tryToRemoveMetaFields(context, false); + // Don't call tryToRemoveMetaFields at all - metadata fields should remain } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential redundancy where tryToRemoveMetaFields(context, false) is called after setProjectVisited(true). However, examining the tryToRemoveMetaFields implementation shows it checks context.isIncludeMetadata() first (lines 658-660), which provides the primary guard. The setProjectVisited(true) call serves as a secondary safeguard. While removing the call could simplify the logic, the current implementation is defensive and not incorrect.	Medium
	Preserve metadata flag during engine fallback When falling back to the legacy engine after a Calcite failure, the includeMetadata flag is lost and not passed to executeWithLegacy. This means users who explicitly requested include_metadata=true will silently get different behavior after fallback. Consider preserving the flag or logging a warning about the behavior change. core/src/main/java/org/opensearch/sql/executor/QueryService.java [216-223] } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { - log.warn("Fallback to V2 query engine since got exception", t); + if (includeMetadata) { + log.warn("Fallback to V2 query engine - include_metadata parameter will be ignored", t); + } else { + log.warn("Fallback to V2 query engine since got exception", t); + } // Legacy engine provides basic metadata support, so fallback is acceptable executeWithLegacy(plan, queryType, listener, Optional.of(t)); } else { propagateCalciteError(t, listener); } } Suggestion importance[1-10]: 7 __ Why: This is a valid concern about losing the includeMetadata flag during fallback to the legacy engine. Adding a conditional warning when includeMetadata=true would help users understand the behavior change. The suggestion improves observability and user experience when fallback occurs, though the impact is limited to fallback scenarios.	Medium
	Warn when unsupported parameter is ignored The includeMetadata parameter is silently ignored when using the legacy engine, which could lead to unexpected behavior for users. Consider logging a warning when includeMetadata=true is specified but the legacy engine is used, or document this limitation clearly to avoid confusion. core/src/main/java/org/opensearch/sql/executor/QueryService.java [116-121] } else { + if (includeMetadata) { + log.warn("include_metadata parameter is not supported by legacy engine and will be ignored"); + } // Legacy engine always includes basic metadata (schema information) // The includeMetadata flag doesn't affect legacy engine behavior since // it already provides column names, types, and aliases in the schema executeWithLegacy(plan, queryType, listener, Optional.empty()); } Suggestion importance[1-10]: 6 __ Why: Adding a warning when includeMetadata=true is used with the legacy engine would improve user experience by making the limitation explicit. However, the comments already document this behavior, and the legacy engine is likely being phased out. The suggestion is valid but has moderate impact since users can discover this through documentation.	Low

Suggestions up to commit c007980

Category	Suggestion	Impact
General	Remove redundant metadata removal call The logic for handling AllFields (include_metadata=true) calls tryToRemoveMetaFields(context, false) which may still remove metadata fields if !context.isProjectVisited() evaluates to true. Since setProjectVisited(true) is called just before, this creates a timing dependency. Consider removing the tryToRemoveMetaFields call entirely for the AllFields case to ensure metadata fields are never removed. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-509] if (allFields instanceof AllFieldsExcludeMeta) { // For AllFieldsExcludeMeta (include_metadata=false), remove nested fields and force exclude // metadata tryToRemoveNestedFields(context); tryToRemoveMetaFields(context, true); // Force exclude metadata fields } else { // For AllFields (include_metadata=true), include metadata fields tryToRemoveNestedFields(context); // Mark as project visited to prevent automatic metadata field removal context.setProjectVisited(true); - // Don't force exclude metadata fields - let them remain - tryToRemoveMetaFields(context, false); + // Don't call tryToRemoveMetaFields at all - metadata fields should remain } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that calling tryToRemoveMetaFields(context, false) after setProjectVisited(true) creates a timing dependency. However, examining the tryToRemoveMetaFields implementation shows it checks context.isIncludeMetadata() first (lines 658-660), which provides the primary protection. Removing the call would simplify the logic and eliminate the timing dependency, making the code more maintainable.	Medium
	Preserve parameter during fallback When falling back to the legacy engine after a Calcite failure, the includeMetadata parameter is lost and not passed to executeWithLegacy. This means users who specified include_metadata=true will silently get different behavior after fallback. Consider preserving the parameter or logging a warning about the behavior change. core/src/main/java/org/opensearch/sql/executor/QueryService.java [216-223] } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { log.warn("Fallback to V2 query engine since got exception", t); + if (includeMetadata) { + log.warn("include_metadata parameter will be ignored in legacy engine fallback"); + } // Legacy engine provides basic metadata support, so fallback is acceptable executeWithLegacy(plan, queryType, listener, Optional.of(t)); } else { propagateCalciteError(t, listener); } } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a valid concern about the includeMetadata parameter being lost during fallback to the legacy engine. Adding a warning would help users understand the behavior change. However, since the legacy engine doesn't support this parameter by design (as documented in the code), this is more of a user communication improvement than a functional bug.	Low
	Warn when parameter is ignored The includeMetadata parameter is silently ignored when using the legacy engine, which could lead to unexpected behavior for users. Consider logging a warning when includeMetadata=true is specified but the legacy engine is used, or document this limitation clearly in the method signature. core/src/main/java/org/opensearch/sql/executor/QueryService.java [116-121] } else { + if (includeMetadata) { + log.warn("include_metadata parameter is not supported by legacy engine and will be ignored"); + } // Legacy engine always includes basic metadata (schema information) // The includeMetadata flag doesn't affect legacy engine behavior since // it already provides column names, types, and aliases in the schema executeWithLegacy(plan, queryType, listener, Optional.empty()); } Suggestion importance[1-10]: 5 __ Why: Adding a warning when includeMetadata=true is used with the legacy engine would improve user experience by making the limitation explicit. However, the existing comment already documents this behavior, and the parameter is intentionally designed to only affect the Calcite engine. The suggestion is valid but represents a minor enhancement rather than a critical issue.	Low

Suggestions up to commit 05e612c

Category	Suggestion	Impact
Possible issue	Prevent unintended metadata field removal The logic for handling AllFields (include_metadata=true) calls tryToRemoveMetaFields(context, false) which may still remove metadata fields based on the isProjectVisited flag. This could lead to inconsistent behavior where metadata fields are removed even when include_metadata=true. Consider not calling tryToRemoveMetaFields at all for the AllFields case to ensure metadata fields are preserved. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-509] if (allFields instanceof AllFieldsExcludeMeta) { // For AllFieldsExcludeMeta (include_metadata=false), remove nested fields and force exclude // metadata tryToRemoveNestedFields(context); tryToRemoveMetaFields(context, true); // Force exclude metadata fields } else { // For AllFields (include_metadata=true), include metadata fields tryToRemoveNestedFields(context); // Mark as project visited to prevent automatic metadata field removal context.setProjectVisited(true); - // Don't force exclude metadata fields - let them remain - tryToRemoveMetaFields(context, false); + // Don't call tryToRemoveMetaFields to ensure metadata fields remain } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential issue where calling tryToRemoveMetaFields(context, false) for AllFields may still remove metadata fields based on the isProjectVisited flag. The improved code removes this call to ensure metadata fields are preserved when include_metadata=true, which aligns with the intended behavior.	Medium
General	Verify backward compatibility of default value The overloaded method defaults includeMetadata to false, which may break backward compatibility for existing callers who expect metadata fields to be included. Verify that this default aligns with the intended behavior for all existing call sites, or consider preserving the previous behavior for backward compatibility. core/src/main/java/org/opensearch/sql/executor/QueryService.java [99-105] public void execute( UnresolvedPlan plan, QueryType queryType, HighlightConfig highlightConfig, ResponseListener<ExecutionEngine.QueryResponse> listener) { + // Preserve backward compatibility by defaulting to false + // Verify this aligns with expected behavior for existing callers execute(plan, queryType, highlightConfig, false, listener); } Suggestion importance[1-10]: 6 __ Why: The suggestion asks to verify backward compatibility when defaulting includeMetadata to false. While this is a valid concern, the PR's default behavior of excluding metadata fields is intentional and documented. The suggestion adds a comment but doesn't change functionality, making it a verification request rather than a code fix.	Low

Suggestions up to commit 8b3962e

Category	Suggestion	Impact
Possible issue	Remove metadata field filtering call The logic for handling AllFields (include_metadata=true) calls tryToRemoveMetaFields(context, false) which may still remove metadata fields based on the isProjectVisited check. This could lead to inconsistent behavior where metadata fields are removed even when include_metadata=true. Consider removing the call to tryToRemoveMetaFields entirely for the AllFields case to ensure metadata fields are always preserved. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-509] if (allFields instanceof AllFieldsExcludeMeta) { // For AllFieldsExcludeMeta (include_metadata=false), remove nested fields and force exclude // metadata tryToRemoveNestedFields(context); tryToRemoveMetaFields(context, true); // Force exclude metadata fields } else { // For AllFields (include_metadata=true), include metadata fields tryToRemoveNestedFields(context); // Mark as project visited to prevent automatic metadata field removal context.setProjectVisited(true); - // Don't force exclude metadata fields - let them remain - tryToRemoveMetaFields(context, false); + // Don't call tryToRemoveMetaFields at all - preserve all metadata fields } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential issue where tryToRemoveMetaFields(context, false) could still remove metadata fields even when include_metadata=true, depending on the isProjectVisited check. Removing this call ensures consistent behavior for metadata inclusion.	Medium
General	Document metadata parameter limitation in fallback When falling back to the legacy engine, the includeMetadata parameter is not passed to executeWithLegacy. This means the user's metadata preference is lost during fallback, potentially causing inconsistent behavior. Verify that the legacy engine respects the metadata setting or document this limitation. core/src/main/java/org/opensearch/sql/executor/QueryService.java [216-222] } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { log.warn("Fallback to V2 query engine since got exception", t); - // Legacy engine provides basic metadata support, so fallback is acceptable + // Note: Legacy engine doesn't support include_metadata parameter + // User's metadata preference may not be honored in fallback executeWithLegacy(plan, queryType, listener, Optional.of(t)); } else { propagateCalciteError(t, listener); } Suggestion importance[1-10]: 6 __ Why: Valid observation that the includeMetadata parameter is not passed to executeWithLegacy. However, the PR already includes a comment explaining that the legacy engine provides basic metadata support. The suggestion to document this limitation more explicitly is helpful but has moderate impact.	Low
	Verify metadata preservation with pagination When pagination is used, the includeMetadata flag is passed to queryService.execute with a Paginate wrapper. Verify that the Paginate plan node correctly preserves and propagates the metadata inclusion behavior through the query execution pipeline, as pagination may affect field projection logic. core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java [106-117] public void execute() { if (pageSize.isPresent()) { + // Ensure Paginate wrapper preserves includeMetadata behavior queryService.execute( new Paginate(pageSize.get(), plan), getQueryType(), highlightConfig, includeMetadata, listener); } else { queryService.execute(plan, getQueryType(), highlightConfig, includeMetadata, listener); } } Suggestion importance[1-10]: 5 __ Why: The suggestion asks to verify that Paginate preserves includeMetadata behavior. While this is a valid concern for testing, the code correctly passes the includeMetadata flag through the execution pipeline. The suggestion is more about verification than a code issue.	Low

Suggestions up to commit b79855b

Category	Suggestion	Impact
General	Verify metadata preservation logic The logic appears inverted. When includeMetadata is true and not forced to exclude, metadata fields should be retained, but the method tryToRemoveMetaFields is designed to remove them. Verify that returning early here correctly preserves metadata fields instead of removing them. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [658-660] +// When include_metadata=true and not forced, skip metadata removal to preserve them if (context.isIncludeMetadata() && !excludeByForce) { - return; + return; // Correctly preserves metadata by not removing } Suggestion importance[1-10]: 7 __ Why: The suggestion asks to verify the logic for metadata preservation, which is reasonable given the complexity of the conditional. However, the logic appears correct: when includeMetadata is true and not forced to exclude, the method returns early to skip removal, thus preserving metadata. The suggestion is valid for verification purposes but doesn't identify an actual bug.	Medium

[ishag4]

Hi @LantaoJin @penghuo @RyanL1997 @Swiddis Could you please review?

[LantaoJin]

Please add integration tests for this enhancement and update documentation (add a new section in endpoint.md

[github-actions]

Persistent review updated to latest commit b79855b

[ishag4]

Hi @LantaoJin @penghuo @RyanL1997 @Swiddis Could you please re-review?

[Swiddis]

One issue, a few suggestions & polish

[Swiddis]

suggestion: It may be worth merging this with HighlightConfig and renaming it to something like QueryConfig (with fieldNames -> highlightFieldNames and toFieldMap -> highlightFieldMap)

Semantically they're similar in that they provide different ways the query might run, and this means we don't now need to be passing two parameters everywhere. It'd overall reduce the one-liner changes of "the same code but add a field" and make it easier to add more config in the future.

[ishag4]

Good suggestion.
I agree a shared QueryConfig (or similar) would likely scale better than continuing to thread individual execution flags through constructors/methods.

We can avoid doing that in this PR mainly to keep the scope focused on include_metadata behavior itself and minimize the amount of unrelated refactoring/review surface.

That said, now that we have both HighlightConfig and includeMetadata, I can definitely see the benefit of consolidating execution-time options before more flags get added.

Happy to follow up with a separate cleanup/refactor PR if that direction makes sense.

[github-actions]

Persistent review updated to latest commit 8b3962e

[github-actions]

Persistent review updated to latest commit 05e612c

[github-actions]

Persistent review updated to latest commit c007980

[ishag4]

Hi @Swiddis @LantaoJin @penghuo, could you please re-review? A few pipelines were failing, and I’ve pushed the necessary fixes. The workflows are now awaiting approval.

[github-actions]

Persistent review updated to latest commit 7194590