Fix PIT context leak in Legacy SQL for non-paginated queries #5009
Conversation
The Legacy SQL engine created PIT contexts for all queries but only cleaned them up for paginated queries (fetch_size > 0). This caused PIT accumulation until the 300 limit was hit. Solution: - Only create PIT when fetch_size > 0 - Add try-catch-finally for proper PIT cleanup - Add null safety check before cursor creation Testing: 9 unit tests + 3 integration tests added Resolves opensearch-project#5002 Signed-off-by: Aaron Alvarez <[email protected]>
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughPrettyFormatRestExecutor was changed to create and manage Point-In-Time (PIT) contexts only for paginated queries (fetchSize > 0). New helpers split pagination vs non-pagination flows; PIT is created only when a cursor will be produced and is deleted when unused or on error. New unit and integration tests validate behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor as PrettyFormatRestExecutor
participant QueryAction
participant PIT as PointInTimeHandler
participant Cursor as DefaultCursor
Client->>Executor: execute SQL (may include fetchSize)
Executor->>QueryAction: explain()
QueryAction-->>Executor: metadata
alt fetchSize > 0 (pagination)
Executor->>PIT: create PIT (keep_alive)
PIT-->>Executor: pit_id
Executor->>QueryAction: search with PIT
else no pagination
Executor->>QueryAction: search without PIT
end
QueryAction-->>Executor: search response (hits, total)
alt shouldCreateCursor (fetchSize>0 and total >= fetchSize)
Executor->>Cursor: create DefaultCursor(pit_id, fetchSize, ...)
Cursor-->>Executor: cursor
else
Executor->>PIT: delete PIT (if created)
PIT-->>Executor: delete ack
end
alt exception and PIT created
Executor->>PIT: delete PIT
PIT-->>Executor: delete ack
end
Executor-->>Client: return response + cursor (or NULL_CURSOR)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)**/*.java📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Files:
⚙️ CodeRabbit configuration file
Files:
integ-test/**/*IT.java📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*IT.java📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Files:
**/test/**/*.java⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
🧬 Code graph analysis (1)integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
🔇 Additional comments (12)
Comment |
aalva500-prog
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java (1)
137-146: Double-deletion of PIT is possible but harmless.If an exception occurs after PIT creation but before
cursorCreated = true, the catch block deletes the PIT and re-throws. Then the finally block also attempts deletion becausecursorCreatedis stillfalse. While this is functionally safe (the second delete would likely 404), consider setting a flag or restructuring to avoid the redundant call.🔎 Proposed fix to avoid double deletion
} catch (Exception e) { if (pit != null) { try { pit.delete(); + pit = null; // Prevent double deletion in finally } catch (RuntimeException cleanupException) { LOG.error("Failed to delete PIT during exception handling", cleanupException); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); } } throw e; } finally {integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java (2)
128-150: Test lacks assertion that PIT count returns to baseline after cursor close.The test verifies cursor management but doesn't assert that
finalPitCountequalsbaselinePitCount, which would confirm proper PIT cleanup. Consider adding an assertion for completeness.🔎 Proposed fix
int finalPitCount = getCurrentPitCount(); System.out.println( String.format( "With proper cursor management: Baseline=%d, Final=%d", baselinePitCount, finalPitCount)); + + assertThat("PIT should be cleaned up after cursor close", finalPitCount, equalTo(baselinePitCount)); }
156-182: TesttestCompareV1AndV2EnginePitBehaviorlacks meaningful assertions.This test only prints PIT counts and verifies that both engines return results, but doesn't assert anything about the PIT behavior difference. Consider adding assertions to validate the expected behavior, or document why the comparison is informational-only.
🔎 Proposed fix with PIT leak assertion
assertTrue("V1 should return results", v1Response.has("datarows")); assertTrue("V2 should return results", v2Response.has("datarows")); + + // Both engines should not leak PITs for non-paginated queries + assertThat("V1 Legacy SQL should not leak PITs", v1Leaked, equalTo(0)); }legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java (2)
99-106: Test duplicates the logic oftestNoPitCreatedWhenFetchSizeIsZero.This test is identical to the previous one. Additionally, to truly test "not specified" behavior,
fetchSize()should returnnull, not0. However, based on the issue flagged inPrettyFormatRestExecutor.java, returningnullwould currently cause an NPE.🔎 Proposed fix to differentiate the test
Once the null-safety fix is applied in the main code, update this test:
@Test public void testNoPitCreatedWhenFetchSizeNotSpecified() throws Exception { - when(sqlRequest.fetchSize()).thenReturn(0); + when(sqlRequest.fetchSize()).thenReturn(null); // Simulates fetch_size not in request executor.execute(client, params, queryAction); verify(searchRequestBuilder, never()).setPointInTime(any(PointInTimeBuilder.class)); }
122-132: Tests lack assertions to verify expected behavior.Both
testFetchSizeSpecifiedButResultsFitInOnePageandtestCursorCreatedWhenResultsExceedFetchSizeexecute without any assertions. While the JavaDoc notes that full verification requires integration testing, consider adding at minimum a verification thatsearchRequestBuilder.get()was called, ensuring the query executed.🔎 Proposed fix to add basic assertions
@Test public void testFetchSizeSpecifiedButResultsFitInOnePage() throws Exception { when(sqlRequest.fetchSize()).thenReturn(100); SearchHits hits = new SearchHits( new SearchHit[] {searchHit}, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F); when(searchResponse.getHits()).thenReturn(hits); - executor.execute(client, params, queryAction); + String result = executor.execute(client, params, queryAction); + + org.junit.Assert.assertNotNull("Query should execute successfully", result); + verify(searchRequestBuilder, times(1)).get(); }Also applies to: 147-157
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java
🧰 Additional context used
📓 Path-based instructions (5)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javainteg-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javainteg-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Use `./gradlew :integ-test:integTest` for integration testing in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
🧬 Code graph analysis (2)
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java (1)
legacy/src/main/java/org/opensearch/sql/legacy/utils/StringUtils.java (1)
StringUtils(17-117)
legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java (3)
legacy/src/main/java/org/opensearch/sql/legacy/query/DefaultQueryAction.java (1)
DefaultQueryAction(55-326)legacy/src/main/java/org/opensearch/sql/legacy/query/SqlOpenSearchRequestBuilder.java (1)
SqlOpenSearchRequestBuilder(13-44)legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java (1)
SqlRequest(22-116)
🔇 Additional comments (5)
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java (1)
91-160: Well-structured PIT lifecycle management with proper cleanup.The try-catch-finally pattern ensures PIT resources are properly cleaned up in all scenarios:
- PIT created only when pagination is requested (
fetchSize > 0)- Cursor takes ownership when created, otherwise cleanup in finally
- Exception handling includes PIT cleanup with metric tracking
This addresses the core issue of PIT leaks for non-paginated queries.
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java (2)
44-72: Good test setup with proper cleanup and index creation.The setup correctly handles the 404 case when the index doesn't exist, creates a proper mapping, and uses bulk indexing with refresh for test data. The test data is realistic and covers the query patterns used in the tests.
86-121: Well-structured test for PIT leak verification.This test properly validates the core fix by:
- Recording baseline PIT count
- Executing multiple queries without fetch_size
- Verifying no PIT leakage occurs
The loop with 10 queries provides good coverage for detecting leaks that might accumulate over time.
legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java (2)
164-187: Comprehensive test forisDefaultCursorlogic.This test covers all three scenarios effectively:
fetchSize = 0: no cursorfetchSize > totalHits: no cursor needed (results fit in one page)fetchSize <= totalHits: cursor neededGood coverage of the decision logic that determines PIT lifecycle ownership.
35-50: Good class-level JavaDoc explaining test purpose and limitations.The documentation clearly explains what's being tested, the limitations due to private method access, and points to integration tests for full validation. This is helpful for future maintainers.
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Aaron Alvarez <[email protected]>
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
Show resolved
Hide resolved
|
Have you also checked in other scenarios if the PIT contexts are closed properly |
@vamsimanohar The PIT context leak only happens in Legacy engine when the index name has special characters like |
…able Signed-off-by: Aaron Alvarez <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java (1)
152-175: Consider simplifying the nested structure.The
getCurrentPitCountmethod has deeply nested conditionals (5 levels). While functional, it could be more readable with early returns or Optional chaining.💡 Optional refactor using early returns
private int getCurrentPitCount() throws IOException { Request statsRequest = new Request("GET", PIT_STATS_ENDPOINT); Response response = client().performRequest(statsRequest); JSONObject stats = new JSONObject(TestUtils.getResponseBody(response)); int totalPits = 0; - if (stats.has("nodes")) { - JSONObject nodes = stats.getJSONObject("nodes"); - for (String nodeId : nodes.keySet()) { - JSONObject node = nodes.getJSONObject(nodeId); - if (node.has("indices")) { - JSONObject indices = node.getJSONObject("indices"); - if (indices.has("search")) { - JSONObject search = indices.getJSONObject("search"); - if (search.has("point_in_time_current")) { - totalPits += search.getInt("point_in_time_current"); - } - } - } - } + if (!stats.has("nodes")) { + return 0; } + + JSONObject nodes = stats.getJSONObject("nodes"); + for (String nodeId : nodes.keySet()) { + JSONObject node = nodes.getJSONObject(nodeId); + if (!node.has("indices")) continue; + + JSONObject indices = node.getJSONObject("indices"); + if (!indices.has("search")) continue; + + JSONObject search = indices.getJSONObject("search"); + if (search.has("point_in_time_current")) { + totalPits += search.getInt("point_in_time_current"); + } + } return totalPits; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorPitTest.java
🧰 Additional context used
📓 Path-based instructions (5)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javainteg-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javainteg-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javainteg-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.javainteg-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.javalegacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Use `./gradlew :integ-test:integTest` for integration testing in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
🧬 Code graph analysis (2)
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java (1)
legacy/src/main/java/org/opensearch/sql/legacy/metrics/Metrics.java (1)
Metrics(13-80)
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java (1)
legacy/src/main/java/org/opensearch/sql/legacy/utils/StringUtils.java (1)
StringUtils(17-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (11)
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java (5)
21-31: LGTM! Well-structured integration test.The class properly extends
SQLIntegTestCase, has clear JavaDoc referencing the related issue, and follows the*IT.javanaming convention. The test index name with dots (test-logs-2025.01.01) is intentional to trigger Legacy engine behavior per the PR context.
32-59: LGTM! Clean test setup with proper resource management.The setup method properly handles index cleanup (gracefully handling 404 responses), creates a well-defined schema, and loads minimal but realistic test data.
61-84: LGTM! Comprehensive test for non-paginated query PIT behavior.This test effectively validates that non-paginated queries (without
fetch_size) do not leak PIT contexts. The baseline comparison approach and multiple iterations provide strong coverage for the fix.
86-106: LGTM! Validates proper PIT lifecycle for paginated queries.This test confirms that PITs created for paginated queries are properly cleaned up when cursors are closed. The assertion on
closeResponse.getBoolean("succeeded")provides additional verification of the close operation.
108-133: LGTM! Effective comparison of V1 and V2 engine PIT behavior.This test validates that both Legacy (V1) and V2 SQL engines properly manage PITs for non-paginated queries. The use of backticks to route queries to different engines is a clean testing approach.
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java (4)
90-106: LGTM! Null-safe routing logic addresses previous NPE concerns.The refactored method properly handles nullable
fetchSizewith an explicit null check (fetchSize != null && fetchSize > 0), addressing the NPE concerns raised in previous reviews. The separation into pagination and non-pagination paths improves code clarity.The
queryAction.explain()call on line 98 is necessary to build theSearchRequestBuilderbefore execution, as confirmed by the author's testing.
108-137: LGTM! Robust PIT lifecycle management with proper error handling.This method properly manages the PIT lifecycle:
- Creates PIT for paginated queries
- Conditionally promotes PIT to cursor or deletes it
- Cleans up PIT on exceptions with metrics tracking
- Addresses previous readability concerns by separating pagination logic
The exception handling is appropriate and ensures PITs don't leak even when errors occur.
139-143: LGTM! Clean implementation for non-paginated queries.This method directly addresses the PIT leak issue by executing queries without creating a PIT when pagination is not requested. The straightforward implementation makes the behavior clear.
145-165: LGTM! Well-designed helper methods with null-safe checks.The
shouldCreateCursormethod properly addresses previous NPE concerns with explicit null checks (fetchSize != null && fetchSize > 0). ThecreateCursorWithPithelper provides clean separation of cursor creation logic.legacy/src/test/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutorTest.java (2)
26-30: LGTM! Clear test documentation with proper cross-references.The JavaDoc effectively clarifies that this test focuses on
shouldCreateCursor()logic while directing readers toPrettyFormatRestExecutorPitTestfor comprehensive PIT lifecycle testing. This separation of concerns improves test maintainability.
46-69: LGTM! Comprehensive boundary condition testing.The test cases effectively validate the
shouldCreateCursorlogic:
- Zero fetch size (no pagination needed)
- Insufficient results for pagination
- Sufficient results for pagination (boundary case)
Test naming follows conventions and assertions are specific and meaningful.
integ-test/src/test/java/org/opensearch/sql/legacy/PointInTimeLeakIT.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Aaron Alvarez <[email protected]>
Signed-off-by: Aaron Alvarez <[email protected]>
Signed-off-by: Aaron Alvarez <[email protected]>
Signed-off-by: Aaron Alvarez <[email protected]>
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-5009-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5bf322ff8b006c94524fdfb3c748ae72c2ac4232
# Push it to GitHub
git push --set-upstream origin backport/backport-5009-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-3.1 3.1
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-3.1
# Create a new branch
git switch --create backport/backport-5009-to-3.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5bf322ff8b006c94524fdfb3c748ae72c2ac4232
# Push it to GitHub
git push --set-upstream origin backport/backport-5009-to-3.1
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-3.1Then, create a pull request where the |
…rch-project#5009) Co-authored-by: Aaron Alvarez <[email protected]> (cherry picked from commit 5bf322f)
…rch-project#5009) Co-authored-by: Aaron Alvarez <[email protected]> (cherry picked from commit 5bf322f) Signed-off-by: Aaron Alvarez <[email protected]>
…rch-project#5009) Co-authored-by: Aaron Alvarez <[email protected]> (cherry picked from commit 5bf322f)
…rch-project#5009) Co-authored-by: Aaron Alvarez <[email protected]> (cherry picked from commit 5bf322f) Signed-off-by: Aaron Alvarez <[email protected]>
5 participants
Description
This PR fixes Point-in-Time (PIT) context leak in the Legacy SQL engine when executing queries without
fetch_sizeparameter.Problem:
The Legacy SQL engine was creating PIT contexts for ALL queries but only cleaning them up when cursors were created (paginated queries with
fetch_size > 0). Non-paginated queries leaked PITs, causing accumulation until the 300 PIT limit was exhausted and queries to fail.Solution:
fetch_size > 0and notnull(pagination requested)Impact:
Related Issues
Resolves #5002
Check List
--signoffor-s.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.