[BugFix] Not between should use range query

[qianheng-aws]

Description

Not between should use range query instead of script query. It's caused by Range<?>::lowerBoundType/upperBoundType will throw NPE if its upper/lower Bound is null, therefore it falls back to script push down.

Related Issues

Resolves #4903

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.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Tests

  • Added test coverage for NOT BETWEEN pushdown explanation to validate query optimization plans.

• Bug Fixes

  • Fixed null bound handling in range predicates to correctly process boundary conditions.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Walkthrough

This pull request adds test coverage for NOT BETWEEN predicate pushdown and fixes the PredicateAnalyzer to properly handle null bounds in range predicates by conditionally applying lower and upper bound constraints.

Changes

Cohort / File(s)	Summary
Test Coverage integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java	New test method testNotBetweenPushDownExplain() validates that NOT BETWEEN queries generate the expected Calcite explain plan with proper pushdown.
Expected Test Output integ-test/src/test/resources/expectedOutput/calcite/explain_not_between_push.yaml	New YAML resource defining the expected logical and physical execution plans for NOT BETWEEN pushdown, including LogicalFilter with range search and CalciteEnumerableIndexScan with PushDownContext.
Predicate Range Handling opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java	Modified between() method to guard lower and upper bound predicate applications with null checks, ensuring range predicates are only applied when bounds exist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title '[BugFix] Not between should use range query' clearly and concisely describes the main change: fixing the NOT BETWEEN operator to use range queries instead of script queries.
Description check	✅ Passed	The description is directly related to the changeset, explaining the root cause (NPE from null bounds) and the fix, with a linked issue reference.
Linked Issues check	✅ Passed	The PR changes satisfy issue #4903: the null-check guards in PredicateAnalyzer prevent NPE on null bounds, enabling range query pushdown for NOT BETWEEN; test coverage and YAML expectations validate this behavior.
Out of Scope Changes check	✅ Passed	All changes are in-scope: the PredicateAnalyzer fix addresses the null-bound NPE issue, the test method validates pushdown behavior for NOT BETWEEN, and the YAML file documents the expected explain plan.

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77633ef and 4fb9e25.

📒 Files selected for processing (3)

• integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
• integ-test/src/test/resources/expectedOutput/calcite/explain_not_between_push.yaml
• opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java

🧰 Additional context used

📓 Path-based instructions (6)

integ-test/src/test/resources/**/*

⚙️ CodeRabbit configuration file

integ-test/src/test/resources/**/*: - Verify test data is realistic and representative

• Check data format matches expected schema
• Ensure test data covers edge cases and boundary conditions

Files:

• integ-test/src/test/resources/expectedOutput/calcite/explain_not_between_push.yaml

**/*.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

**/*.java: Use PascalCase for class names (e.g., QueryExecutor)
Use camelCase for method and variable names (e.g., executeQuery)
Use UPPER_SNAKE_CASE for 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
Prefer Optional<T> for nullable returns in Java
Avoid unnecessary object creation in loops
Use StringBuilder for 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:

• integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
• opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.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:

• integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
• opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.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/calcite/remote/CalciteExplainIT.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/calcite/remote/CalciteExplainIT.java

**/*IT.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

Name integration tests with *IT.java suffix in OpenSearch SQL

Files:

• integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.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/calcite/remote/CalciteExplainIT.java

**/calcite/**/*.java

⚙️ CodeRabbit configuration file

**/calcite/**/*.java: - Follow existing Calcite integration patterns

• Verify RelNode visitor implementations are complete
• Check RexNode handling follows project conventions
• Validate SQL generation is correct and optimized
• Ensure Calcite version compatibility
• Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
• Document any Calcite-specific workarounds
• Test compatibility with Calcite version constraints

Files:

• integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

🧠 Learnings (2)

📚 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/resources/expectedOutput/calcite/explain_not_between_push.yaml
• integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

📚 Learning: 2025-12-29T05:32:03.491Z

Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.

Applied to files:

• integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

⏰ 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: security-it-linux (25)
• GitHub Check: security-it-linux (21)
• GitHub Check: build-linux (21, integration)
• GitHub Check: build-linux (25, integration)
• GitHub Check: build-linux (21, unit)
• GitHub Check: build-linux (21, doc)
• GitHub Check: bwc-tests-full-restart (21)
• GitHub Check: build-linux (25, unit)
• GitHub Check: build-linux (25, doc)
• GitHub Check: bwc-tests-rolling-upgrade (21)
• GitHub Check: bwc-tests-rolling-upgrade (25)
• GitHub Check: bwc-tests-full-restart (25)
• GitHub Check: build-windows-macos (macos-14, 21, unit)
• GitHub Check: build-windows-macos (macos-14, 25, integration)
• GitHub Check: build-windows-macos (macos-14, 25, doc)
• GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
• 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 (macos-14, 21, doc)
• GitHub Check: build-windows-macos (macos-14, 21, integration)
• GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
• GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
• GitHub Check: security-it-windows-macos (windows-latest, 25)
• GitHub Check: security-it-windows-macos (windows-latest, 21)
• GitHub Check: security-it-windows-macos (macos-14, 21)
• GitHub Check: security-it-windows-macos (macos-14, 25)
• GitHub Check: test-sql-cli-integration (21)
• GitHub Check: CodeQL-Scan (java)

🔇 Additional comments (3)

opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java (1)

1427-1438: LGTM! Correct fix for NPE with unbounded ranges.

The null guards correctly prevent NPE when Range<?> has null bounds. For NOT BETWEEN predicates, Calcite generates ranges like (-∞..30) and (39..+∞) where bounds are null, and calling boundType() on null would throw NPE. The fix ensures range queries are generated only for present bounds.

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (1)

2311-2320: LGTM! Well-structured test for NOT BETWEEN pushdown.

The test follows existing patterns, correctly enables pushdown, and verifies the explain plan matches the expected range query output. The comment properly references issue #4903.

Based on learnings, consider verifying SQL generation and optimization paths by running the test to ensure the range query is correctly generated for NOT BETWEEN predicates.

integ-test/src/test/resources/expectedOutput/calcite/explain_not_between_push.yaml (1)

1-8: LGTM! Expected output correctly represents NOT BETWEEN as range query.

The explain plan correctly shows:

• Logical: Sarg[(-∞..30), (39..+∞)] representing NOT BETWEEN 30 AND 39
• Physical: Bool query with two range clauses (age < 30 OR age > 39) with proper bound handling (from:null for unbounded lower, to:null for unbounded upper)

This confirms the fix enables range query pushdown instead of falling back to script queries.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}