[BugFix] Return all columns (struct and nested fields) listed when using head

[quangdutran]

Description

When both a struct parent and the nested ones are projected in the query, appending | head must not drop the nested.

Explanation

Query with head has a Project wrapper like Project(Head (Project([agent.id, agent.name, agent])))

// source=big5 | fields agent.id agent.name agent | head

Seems like outer Project hits tryToRemoveNestedFields so nested fields are removed. Without head, there is no wrapper, hence nested fields stay.

tryToRemoveMetaFields has one run condition: There is no other project ever visited in the main query
So I try to apply the same with tryToRemoveNestedFields

Related Issues

Resolves #5507

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.

[quangdutran]

Test result:

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 117e7cc)

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]

PR Code Suggestions ✨

Latest suggestions up to 117e7cc
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add validation for context state The condition logic may be fragile if context.isProjectVisited() can be true in unexpected scenarios. Consider adding validation to ensure the context state is consistent with the intended behavior, or add logging to track when nested field removal is skipped. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [519-523] if (!(allFields instanceof AllFieldsExcludeMeta) && !context.isProjectVisited()) { // Should not remove nested fields for AllFieldsExcludeMeta // and when no explicit project has already curated the schema tryToRemoveNestedFields(context); +} else if (context.isProjectVisited()) { + // Log or validate that skipping nested field removal is intentional } Suggestion importance[1-10]: 3 __ Why: The suggestion asks to add logging or validation for when nested field removal is skipped, which is a minor improvement for debugging purposes. However, it doesn't address a critical issue and the improved_code adds an else-if block that only contains a comment, which provides minimal value. The condition logic appears intentional based on the PR context.	Low

---

Previous suggestions

Suggestions up to commit fcfa34a

Category	Suggestion	Impact
General	Fix typo in test method name The test method name contains a typo: "Struck" should be "Struct". This typo could cause confusion when reading test reports or searching for struct-related tests. Rename the method to testStructFieldAndSubFieldWithHead for clarity. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java [239-244] @Test -public void testStruckFieldAndSubFieldWithHead() throws IOException { +public void testStructFieldAndSubFieldWithHead() throws IOException { JSONObject result = executeQuery( String.format("source=%s | fields city.name, city | head", TEST_INDEX_DEEP_NESTED)); verifySchema(result, schema("city.name", "string"), schema("city", "struct")); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a typo in the method name testStruckFieldAndSubFieldWithHead where "Struck" should be "Struct". This improves code readability and maintainability, though it's a minor issue that doesn't affect functionality.	Low
	Add data row verification to test The test only verifies the schema but doesn't verify the actual data rows returned. Add verifyDataRows to ensure the query returns expected data, not just the correct schema structure. This would make the test more comprehensive and catch potential data retrieval issues. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java [239-244] @Test public void testStruckFieldAndSubFieldWithHead() throws IOException { JSONObject result = executeQuery( String.format("source=%s | fields city.name, city | head", TEST_INDEX_DEEP_NESTED)); verifySchema(result, schema("city.name", "string"), schema("city", "struct")); + verifyDataRows(result); } Suggestion importance[1-10]: 3 __ Why: While adding verifyDataRows could make the test more comprehensive, the suggestion provides an incomplete implementation (calling verifyDataRows(result) without expected data rows). The current test may intentionally focus only on schema verification, making this a marginal improvement without proper implementation details.	Low

[Swiddis]

from doctest and integ test failures it looks like this is also causing structs to be expanded in the case of fields *, which I think is fine? Sub-fields are technically part of the result set described by that doc so it makes sense, and it keeps the behavior consistent. Just need to update the tests

Unit test action is failing due to spotless

[github-actions]

Persistent review updated to latest commit 117e7cc

[quangdutran]

Yes @Swiddis I have refactored the tests