Fix SemanticCheckException not thrown

[sunil9977]

Issue #917 -
Fix SemanticCheckException not thrown for invalid field in nested function

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 12d55fe)

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 Isolated Environment The pushIsolated() method creates a new TypeEnvironment with a null parent. This means any lookup that traverses the parent chain will fail or behave differently than before. It should be validated that all callers of peek() and resolve() after pushIsolated() do not inadvertently require access to parent environment entries (e.g., for subqueries or nested expressions that still need outer scope resolution). public void pushIsolated() { environment = new TypeEnvironment(null); } Missing pop() The previous context.push() was presumably paired with a corresponding context.pop() elsewhere. The new context.pushIsolated() should also be paired with a pop() to avoid leaking the isolated environment onto the stack for subsequent operations. Verify that the environment pushed here is properly cleaned up after the project node is fully processed. context.pushIsolated(); TypeEnvironment newEnv = context.peek();

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 12d55fe
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Isolated push breaks environment restoration on pop The pushIsolated method replaces the current environment without saving a reference to it, making it impossible to pop() back to the previous environment later. The pop() method relies on getParent() to restore the previous environment, but since the new isolated environment has null as parent, calling pop() after pushIsolated() will set environment to null, losing the prior context entirely. Consider storing the previous environment so it can be restored, or documenting that pop() must not be called after pushIsolated(). core/src/main/java/org/opensearch/sql/analysis/AnalysisContext.java [76-78] public void pushIsolated() { - environment = new TypeEnvironment(null); + // Store previous environment as a non-accessible parent reference + // so that pop() can still restore it correctly + environment = new TypeEnvironment(null) { + private final TypeEnvironment previous = environment; + @Override + public TypeEnvironment getParent() { + return previous; + } + }; } Suggestion importance[1-10]: 6 __ Why: The concern is valid - calling pop() after pushIsolated() will set environment to null since the isolated environment has no parent. However, the PR's intent appears to be specifically to create an isolated environment without parent access (as stated in the Javadoc), and the improved_code uses an anonymous class override which is a somewhat hacky solution. The actual behavior concern is real but the fix approach is questionable.	Low

---

Previous suggestions

Suggestions up to commit e6e9737

Category	Suggestion	Impact
Possible issue	Preserve previous environment for correct pop restoration The pushIsolated method creates a new environment with null as the parent, but it doesn't save the previous environment anywhere. This means the previous environment is lost and cannot be restored via pop(), since pop() will set environment to null (the parent of the new isolated env). Consider storing the previous environment as a field or passing it differently so pop() can correctly restore it. core/src/main/java/org/opensearch/sql/analysis/AnalysisContext.java [78-80] public void pushIsolated() { - environment = new TypeEnvironment(null); + environment = new TypeEnvironment(null) { + private final TypeEnvironment previousEnv = environment; + @Override + public TypeEnvironment getParent() { + return previousEnv; + } + }; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that pushIsolated() with null parent will cause pop() to set environment to null, potentially breaking subsequent operations. However, the proposed fix using an anonymous class override is unconventional and may conflict with the intended "isolated" semantics of the method. The issue is real but the fix approach is debatable.	Medium

[Swiddis]

Spotless check is failing in unit CI (./gradlew spotlessApply)

[opensearch-trigger-bot]

This PR is stalled because it has been open for 2 weeks with no activity.

[Swiddis]

Bump: CI failure, if not updated by the next stalled cycle will close

[github-actions]

Persistent review updated to latest commit 12d55fe

[opensearch-trigger-bot]

This PR is stalled because it has been open for 2 weeks with no activity.

[ahkcs]

@sunil9977 are you still tracking this?

[sunil9977]

Yes, I'm looking for the reviewers.