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 282b487)

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 The includeMetadata field is copied from parent to child context in the lambda constructor, but the parent's includeMetadata may not be initialized yet when the child context is created. If setIncludeMetadata is called on the parent after child creation, the child will have stale state. This could cause metadata fields to be incorrectly excluded or included in lambda expressions depending on initialization order. this.includeMetadata = parent.includeMetadata; // Preserve parent's metadata setting Possible Issue The constructor at line 45 calls this(...) with includeMetadata hardcoded to false, but the old code called this(...) with null for highlightConfig. If any caller relied on the previous constructor signature where highlightConfig was implicitly null, they now get includeMetadata=false as well. This changes behavior silently for existing callers who may have expected different defaults. this(queryId, queryType, plan, queryService, listener, null, false);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 282b487

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Warn when metadata parameter is ignored The includeMetadata parameter is silently ignored for the V2 engine without any validation or warning. If a user explicitly sets include_metadata=true but the query uses the V2 engine, they won't receive any indication that their request is being ignored. Consider logging a warning or validating this scenario. core/src/main/java/org/opensearch/sql/executor/QueryService.java [123-126] } else { // The V2 engine has no notion of metadata fields, so includeMetadata is ignored there. + if (includeMetadata) { + log.warn("include_metadata parameter is not supported by the V2 engine and will be ignored"); + } executeWithLegacy(plan, queryType, listener, Optional.empty()); } Suggestion importance[1-10]: 5 __ Why: This is a reasonable usability improvement that would help users understand when their include_metadata parameter is being ignored by the V2 engine. The suggestion correctly identifies that silent ignoring could be confusing, and adding a warning would improve the user experience. However, it's not critical functionality.	Low
	Review metadata inheritance in lambda contexts The includeMetadata flag is copied from parent context in lambda contexts, but this may not be the intended behavior. Lambda expressions typically operate on already-projected data and shouldn't independently control metadata inclusion. Consider whether lambda contexts should inherit this setting or if it should be reset to a default value. core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java [154] -this.includeMetadata = parent.includeMetadata; // Preserve parent's metadata setting +// Lambda contexts inherit metadata setting from parent for consistency +this.includeMetadata = parent.includeMetadata; Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid design consideration about whether lambda contexts should inherit includeMetadata from their parent. However, the improved code is essentially identical to the existing code (just adds a comment), and the concern is speculative without evidence of actual issues. The inheritance behavior appears intentional based on the comment already present.	Low
	Clarify null handling in metadata getter The method returns false when jsonContent is null, but this could mask configuration errors. If the request is malformed or missing required content, returning a default value silently may lead to unexpected behavior. Consider whether this null check is appropriate or if it should throw an exception for invalid requests. ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java [135-140] public boolean getIncludeMetadata() { if (jsonContent == null) { + // Return default when no JSON content is provided (e.g., GET requests) return false; } return jsonContent.optBoolean(INCLUDE_METADATA_FIELD, false); } Suggestion importance[1-10]: 2 __ Why: The improved code only adds a comment to explain existing behavior, which doesn't change functionality. The null check appears appropriate for handling cases where no JSON content is provided (like GET requests), and the suggestion doesn't identify an actual bug or significant improvement opportunity.	Low

---

Previous suggestions

Suggestions up to commit b718cb6

Category	Suggestion	Impact
General	Handle nested field removal consistently The logic for AllFieldsExcludeMeta skips nested field removal, which may cause inconsistent behavior. When include_metadata=false, nested fields should still be removed to maintain consistency with the default behavior. Consider calling tryToRemoveNestedFields(context) for both branches when !context.isProjectVisited(). core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [533-545] if (allFields instanceof AllFieldsExcludeMeta) { - // For AllFieldsExcludeMeta (include_metadata=false), should not remove nested fields + // For AllFieldsExcludeMeta (include_metadata=false), remove nested fields if not visited + if (!context.isProjectVisited()) { + tryToRemoveNestedFields(context); + } tryToRemoveMetaFields(context, true); // Force exclude metadata fields } else { // For AllFields (include_metadata=true), include metadata fields if (!context.isProjectVisited()) { 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); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential inconsistency where AllFieldsExcludeMeta skips nested field removal. Adding tryToRemoveNestedFields(context) for AllFieldsExcludeMeta when !context.isProjectVisited() would ensure consistent behavior across both branches and maintain the expected default behavior.	Medium
	Extract default metadata flag constant The default value for includeMetadata is hardcoded as false in this overload. If the default behavior changes in the future, this could lead to inconsistencies. Consider extracting the default value to a constant to ensure consistency across the codebase. core/src/main/java/org/opensearch/sql/executor/QueryService.java [106-112] +private static final boolean DEFAULT_INCLUDE_METADATA = false; + public void execute( UnresolvedPlan plan, QueryType queryType, HighlightConfig highlightConfig, ResponseListener<ExecutionEngine.QueryResponse> listener) { - execute(plan, queryType, highlightConfig, false, listener); + execute(plan, queryType, highlightConfig, DEFAULT_INCLUDE_METADATA, listener); } Suggestion importance[1-10]: 5 __ Why: The suggestion improves maintainability by extracting the hardcoded false default value into a named constant. This makes the default behavior more explicit and easier to change consistently across the codebase if needed in the future.	Low

Suggestions up to commit 7194590

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

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

[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

[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

[github-actions]

Persistent review updated to latest commit b718cb6

[mengweieric]

@ishag4 please check failing CIs

[github-actions]

Persistent review updated to latest commit 282b487

[ishag4]

Hi @mengweieric Can you please re-trigger the CIs?

[ishag4]

Hi @mengweieric @Swiddis @LantaoJin @penghuo Could you please review and approve this PR?