Enhance doc and error message handling for `bins` on time-related fields by ahkcs · Pull Request #4713 · opensearch-project/sql

ahkcs · 2025-10-31T22:07:51Z

Description

Summary

This PR adds clear error handling and documentation for the known limitation that the bins parameter on timestamp fields requires pushdown to be enabled.

Related Issues

Resolves #[BUG] PPL bin command fails with param bins on time-related fields #4578

ahkcs · 2025-11-05T22:19:04Z

unit tests in CI seems flaky

noCharger · 2025-11-06T07:20:06Z


    // Create validated binnable field (validates that field is numeric or time-based)
-    // Note: bins parameter works with both numeric and time-based fields
+    // Note: bins parameter on time-based fields requires pushdown to be enabled


Nit: Describe the default behavior and the configuration that controls pushdown.

Updated comments and documentation

opensearch-trigger-bot · 2025-12-04T15:22:40Z

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

…d fields with pushdown disabled Signed-off-by: Kai Huang <ahkcs@amazon.com>

Signed-off-by: Kai Huang <ahkcs@amazon.com>

coderabbitai · 2025-12-09T22:31:28Z

📝 Walkthrough

Summary by CodeRabbit

Bug Fixes
- Improved error messaging when using the bins parameter with timestamp fields to provide clearer guidance on pushdown requirements.
Documentation
- Added documentation for bins parameter constraints with timestamp fields, including pushdown enablement prerequisites and proper aggregation bucket configuration.

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

Walkthrough

This PR adds error handling for WIDTH_BUCKET plan preparation failures in Calcite, introduces limitations documentation for the bins command with timestamp fields, and adds test coverage for pushdown-disabled scenarios. Changes span error detection logic, documentation updates, and test utilities.

Changes

Cohort / File(s)	Summary
Error Handling & Plan Preparation `core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java`	Added SQLException detection for WIDTH_BUCKET plan preparation errors; throws UnsupportedOperationException with guidance to enable pushdown and use timestamp fields as aggregation buckets
Documentation Updates `docs/user/ppl/cmd/bin.md`, `core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java`	Expanded documentation detailing bins parameter limitations for timestamp fields, including pushdown enablement requirements and aggregation bucket constraints
Test Coverage `integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java`, `integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java`	Added test method for pushdown-disabled failure scenarios; added test guard method `enabledOnlyWhenPushdownIsDisabled()` for symmetric precondition checks

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Duplicate test method declaration: The testBinsOnTimeFieldWithPushdownDisabled_ShouldFail() method appears twice in CalciteBinCommandIT.java—verify if this is intentional or requires removal
Error message alignment: Cross-check that the exception message in CalciteToolsHelper.java matches the documentation guidance in bin.md
Import consistency: Review the dual import of assertThrows from both org.junit.Assert and org.junit.jupiter.api.Assertions in CalciteBinCommandIT.java

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

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

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: enhancing documentation and error message handling for the `bins` parameter when used with time-related fields.
Description check	✅ Passed	The description is clearly related to the changeset, explaining that the PR adds error handling and documentation for the `bins` parameter limitation on timestamp fields.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

Signed-off-by: Kai Huang <ahkcs@amazon.com>

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java (1)

31-35: Align comment terminology with user-facing docs

The comment refers to “time-based fields” while the error message and user docs call out “timestamp fields” specifically. To avoid confusion for maintainers, consider tightening this to “timestamp fields” (or explicitly listing supported time types) so all layers use the same wording for this limitation.
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
363-373: Preserve underlying SQLException when wrapping and document workaround

The targeted check on "Error while preparing plan" + "WIDTH_BUCKET" is a reasonable Calcite-specific workaround with a good user-facing message. Two small improvements:

Wrap the original exception as the cause so debugging retains full context:
-          throw new UnsupportedOperationException(
-              "The 'bins' parameter on timestamp fields requires: (1) pushdown to be enabled"
-                  + " (controlled by plugins.calcite.pushdown.enabled, enabled by default), and"
-                  + " (2) the timestamp field to be used as an aggregation bucket (e.g., 'stats"
-                  + " count() by @timestamp').");
+          throw new UnsupportedOperationException(
+              "The 'bins' parameter on timestamp fields requires: (1) pushdown to be enabled"
+                  + " (controlled by plugins.calcite.pushdown.enabled, enabled by default), and"
+                  + " (2) the timestamp field to be used as an aggregation bucket (e.g., 'stats"
+                  + " count() by @timestamp').",
+              e);
Optionally add a brief comment above this block referencing the related Calcite/binning limitation (e.g., issue ID) so future maintainers know why we’re pattern-matching on the message.

📜 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 5f963a0 and 2d9da1c.

📒 Files selected for processing (5)

core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1 hunks)
core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java (1 hunks)
docs/user/ppl/cmd/bin.md (1 hunks)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java (2 hunks)
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (1 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

**/*.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/ppl/PPLIntegTestCase.java
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java
core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java

⚙️ CodeRabbit configuration file

**/*.java: - 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 methods are under 20 lines with single responsibility

Verify 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/ppl/PPLIntegTestCase.java
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java
core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java

**/test/**/*.java

⚙️ CodeRabbit configuration file

**/test/**/*.java: - Verify test coverage for new business logic

Check test naming follows conventions (*Test.java for unit, *IT.java for integration)

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/ppl/PPLIntegTestCase.java
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java

**/ppl/**/*.java

⚙️ CodeRabbit configuration file

**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases

Check AST generation for new syntax

Ensure corresponding AST builder classes are updated

Validate edge cases and boundary conditions

Files:

integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java

**/calcite/**/*.java

⚙️ CodeRabbit configuration file

**/calcite/**/*.java: - Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor

Verify SQL generation and optimization paths

Document any Calcite-specific workarounds

Test compatibility with Calcite version constraints

Files:

core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java
core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.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/CalciteBinCommandIT.java

⚙️ CodeRabbit configuration file

integ-test/**/*IT.java: - 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/CalciteBinCommandIT.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/CalciteBinCommandIT.java

🧠 Learnings (7)

📓 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