[BugFix] Keep metadata fields hidden per input of append, appendpipe, appendcol, union and lookup

[LantaoJin]

Description

With the Calcite engine, a fields in one input of a command that combines inputs exposed the metadata fields (_id, _index, _score, _maxscore, _sort, _routing) of the other input, e.g. source=A | append [ source=B | fields x ] returned A's metadata columns. The cause is that CalcitePlanContext.isProjectVisited is one flag for the whole statement: any fields sets it, and the final project removes metadata fields only while it is unset.

This PR adds CalciteRelNodeVisitor.visitInputsWithOwnMetaFields, used by append, appendpipe, appendcol and union. It visits each input with the flag cleared. If some inputs have an explicit project and others do not, it removes the metadata fields of the others with the existing non-forced tryToRemoveMetaFields, so include_metadata=true still keeps them. If no input has a project, nothing changes: the final project removes the metadata fields as before, so append [ ... ] | fields _id still works. The flag afterwards has the same value as before this change. appendcol's two copies of the row_number code are now one helper, projectPlusRowNumber.

A lookup without an output list provided every field of the lookup index, its metadata fields included. They are now removed unless they are mapping fields, which matches the output-list path (it projects only the mapping and output fields) and the way join removes the metadata of its right side.

The same change fixes SQL SELECT id, msg FROM t UNION ALL SELECT * FROM t in the unified query API, which failed with Cannot compute compatible row type for arguments to set op because the SELECT * input kept _id.

Behaviour change: when one input has a fields and another does not, the metadata fields of the input without fields can no longer be referenced after the command, as is already the case after join and multisearch.

Testing:

• CalcitePPLMetadataFieldsTest (17 plan tests, over a table with _id, _index and _score columns) covers each command in both directions, nested append, a metadata field selected on one side, lookup by a metadata field, include_metadata=true, and the row alignment of appendcol. CalcitePPLAppendPipeTest.testAppendPipeInJoinSubsearch checks that appendpipe leaves one node on the RelBuilder stack. UnifiedQueryPlannerSqlV2Test.testUnionAllSelectStarAfterExplicitColumns covers the SQL case.
• New methods in CalcitePPLAppendCommandIT, and the REST API test issues/metadata_fields_multi_input.yml (6 cases).
• With core/ at main, 11 of the 17 plan tests, the SQL test, all 6 REST API cases and the new IT method fail. With the fix, :core:test, :ppl:test, :opensearch:test, :sql:test and :api:test pass with 0 failures. The append, appendpipe, appendcol, union, multisearch, include-metadata, lookup and graphlookup IT classes also pass, with and without pushdown.

Not changed: once the flag is set, the final project also skips tryToRemoveNestedFields, so in the same mixed case an input without fields may still return flattened duplicates such as obj.a next to obj.

Related Issues

Resolves #5883

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 923e800)

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 In visitInputsWithOwnMetaFields, if inputsWithProject.isEmpty() is true (meaning no input has a project), the method sets context.setProjectVisited(projectVisited || !inputsWithProject.isEmpty()) which evaluates to context.setProjectVisited(projectVisited || false), preserving the original value. However, the loop that removes metadata fields only runs when !inputsWithProject.isEmpty() is true. This means if all inputs lack projects but projectVisited was already true from earlier in the statement, metadata fields won't be removed from any input, yet projectVisited remains true, potentially causing the final project to skip metadata removal. This could expose metadata fields unexpectedly when a fields command appears before a multi-input command where no input has its own fields. private static List<RelNode> visitInputsWithOwnMetaFields( List<Runnable> inputVisitors, CalcitePlanContext context) { boolean projectVisited = context.isProjectVisited(); List<RelNode> inputs = new ArrayList<>(); BitSet inputsWithProject = new BitSet(); for (Runnable inputVisitor : inputVisitors) { context.setProjectVisited(false); inputVisitor.run(); inputsWithProject.set(inputs.size(), context.isProjectVisited()); inputs.add(context.relBuilder.build()); } if (!inputsWithProject.isEmpty()) { context.setProjectVisited(false); for (int i = 0; i < inputs.size(); i++) { if (!inputsWithProject.get(i)) { context.relBuilder.push(inputs.get(i)); tryToRemoveMetaFields(context, false); inputs.set(i, context.relBuilder.build()); } } } context.setProjectVisited(projectVisited || !inputsWithProject.isEmpty()); return inputs; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 923e800
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix BitSet emptiness check The condition !inputsWithProject.isEmpty() is incorrect for a BitSet. Use inputsWithProject.cardinality() > 0 to check if any bit is set, as isEmpty() checks if the BitSet has zero length, not if all bits are false. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [770] private static List<RelNode> visitInputsWithOwnMetaFields( List<Runnable> inputVisitors, CalcitePlanContext context) { boolean projectVisited = context.isProjectVisited(); List<RelNode> inputs = new ArrayList<>(); BitSet inputsWithProject = new BitSet(); for (Runnable inputVisitor : inputVisitors) { context.setProjectVisited(false); inputVisitor.run(); inputsWithProject.set(inputs.size(), context.isProjectVisited()); inputs.add(context.relBuilder.build()); } - if (!inputsWithProject.isEmpty()) { + if (inputsWithProject.cardinality() > 0) { context.setProjectVisited(false); for (int i = 0; i < inputs.size(); i++) { if (!inputsWithProject.get(i)) { context.relBuilder.push(inputs.get(i)); tryToRemoveMetaFields(context, false); inputs.set(i, context.relBuilder.build()); } } } - context.setProjectVisited(projectVisited || !inputsWithProject.isEmpty()); + context.setProjectVisited(projectVisited || inputsWithProject.cardinality() > 0); return inputs; } Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical bug. BitSet.isEmpty() checks if the BitSet has zero length (no bits allocated), not if all bits are false. Using cardinality() > 0 is the correct way to check if any bit is set, which is essential for the logic determining whether to remove metadata fields.	High

---

Previous suggestions

Suggestions up to commit 892cb7e

Category	Suggestion	Impact
Possible issue	Fix BitSet emptiness check The condition !inputsWithProject.isEmpty() is incorrect for a BitSet. Use inputsWithProject.cardinality() > 0 to check if any bit is set, as isEmpty() checks if the BitSet has zero capacity, not whether any bits are true. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [770-781] private static List<RelNode> visitInputsWithOwnMetaFields( List<Runnable> inputVisitors, CalcitePlanContext context) { boolean projectVisited = context.isProjectVisited(); List<RelNode> inputs = new ArrayList<>(); BitSet inputsWithProject = new BitSet(); for (Runnable inputVisitor : inputVisitors) { context.setProjectVisited(false); inputVisitor.run(); inputsWithProject.set(inputs.size(), context.isProjectVisited()); inputs.add(context.relBuilder.build()); } - if (!inputsWithProject.isEmpty()) { + if (inputsWithProject.cardinality() > 0) { context.setProjectVisited(false); for (int i = 0; i < inputs.size(); i++) { if (!inputsWithProject.get(i)) { context.relBuilder.push(inputs.get(i)); tryToRemoveMetaFields(context, false); inputs.set(i, context.relBuilder.build()); } } } - context.setProjectVisited(projectVisited || !inputsWithProject.isEmpty()); + context.setProjectVisited(projectVisited || inputsWithProject.cardinality() > 0); return inputs; } Suggestion importance[1-10]: 10 __ Why: This is a critical bug. BitSet.isEmpty() checks if the BitSet has zero capacity, not whether any bits are set. The correct method is cardinality() > 0 to check if any bits are true. This bug would cause incorrect behavior in metadata field handling for multi-input commands.	High

[codecov]

Codecov Report

❌ Patch coverage is 0% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.82%. Comparing base (4d98393) to head (923e800).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
.../opensearch/sql/calcite/CalciteRelNodeVisitor.java	0.00%	43 Missing ⚠️
...ensearch/sql/calcite/utils/JoinAndLookupUtils.java	0.00%	9 Missing ⚠️

❌ Your project check has failed because the head coverage (63.82%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5884      +/-   ##
============================================
- Coverage     63.87%   63.82%   -0.06%     
- Complexity     9091     9093       +2     
============================================
  Files           962      962              
  Lines         41033    41074      +41     
  Branches       4668     4675       +7     
============================================
+ Hits          26211    26214       +3     
- Misses        13944    13982      +38     
  Partials        878      878              

Flag	Coverage Δ
sql-engine	63.82% <0.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[penghuo]

lookup silently drops lookup‑side metadata under include_metadata=true.

This new else branch strips the lookup side's metadata unconditionally — it never checks context.isIncludeMetadata(). The other four commands routed through visitInputsWithOwnMetaFields honor the opt‑in via tryToRemoveMetaFields(context, /*force=*/false), which early‑returns on isIncludeMetadata(). Lookup bypasses that helper, so the two policies disagree inside the same PR.

Dataset. Two indices. worker holds employees — id, name, occupation, country, salary. work_information holds department records keyed by uid matching worker.id — uid, name, department, occupation. Both carry the usual OpenSearch metadata: _id, _index, _score, _maxscore, _sort, _routing.

Query. Enrich each worker with the matching department record; explicitly opt into metadata from both sides.

POST /_plugins/_ppl?include_metadata=true
{ "query": "source=worker | LOOKUP work_information uid AS id" }

Expected. Two groups of metadata columns, one per side.

Actual. Only one group — the source side's. The lookup side's _id, _index, _score, _maxscore, _sort, _routing are silently dropped. No error, no warning.

Consistency check. With include_metadata=true and a fields on one side, append, appendpipe, union, appendcol all retain both legs' metadata; only lookup drops it. Users will reasonably assume uniform behavior across the five commands.

Why it matters. include_metadata=true is the opt‑in for callers who need these columns — dedup by _id after enrichment, audit which _index a joined row came from. Half of them quietly disappearing turns the opt‑in into a half‑open door.

No existing or new test covers lookup + include_metadata=true. Please add one asserting both sides' metadata survive.

[LantaoJin]

I compared include_metadata=true on main and on this branch with plan tests over two tables with distinct _id/_index values. Neither keeps two groups: both sides name their metadata _id, _index, …, so the output holds one column per name.

Query (include_metadata=true unless noted)	main	This PR
source=worker | lookup work_information uid AS id	work_information's _id/_index/_score, null on unmatched rows; the worker's own metadata is lost	The worker's _id/_index/_score on every row
source=worker | lookup work_information uid AS id APPEND occupation	The worker's	The worker's (unchanged)
source=worker | lookup work_information uid AS id | fields id, _id (without include_metadata)	work_information's _id, null on unmatched rows	The worker's _id
source=worker | join on worker.id = work_information.uid work_information	No metadata from either side	No metadata from either side (unchanged)
source=worker | lookup work_information uid AS id OUTPUT _id AS dept_doc_id, _index AS dept_index	Same as this PR (the output-list path is unchanged)	The worker's _id/_index/_score plus dept_doc_id and dept_index

With main's behavior, dedup by _id after enrichment collapses every unmatched worker into one row (_id = null). join removes both sides because tryToRemoveMetaFields force-strips join inputs to keep their output schemas unambiguous.

I'll add tests for lookup with include_metadata=true that pin this, and state the include_metadata case in the Javadoc. If you'd rather have include_metadata=true keep the lookup side's metadata, I can switch it, but it would overwrite the source rows' _id again.

[github-actions]

Persistent review updated to latest commit 923e800