diff --git a/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java b/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java index 6513f51a5a..b1b2efc966 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java +++ b/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java @@ -355,6 +355,17 @@ public RelNode visit(TableScan scan) { final RelRunner runner = connection.unwrap(RelRunner.class); return runner.prepareStatement(rel); } catch (SQLException e) { + // Detect if error is due to window functions in unsupported context (bins on time fields) + String errorMsg = e.getMessage(); + if (errorMsg != null + && errorMsg.contains("Error while preparing plan") + && errorMsg.contains("WIDTH_BUCKET")) { + 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 Util.throwAsRuntime(e); } } diff --git a/core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java b/core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java index 7422a26f0b..ea1ca6a15f 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java +++ b/core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java @@ -28,7 +28,11 @@ public RexNode createExpression( CountBin countBin = (CountBin) node; // 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: + // 1. Pushdown to be enabled (controlled by plugins.calcite.pushdown.enabled, enabled by + // default) + // 2. The time-based field to be used as an aggregation bucket + // (e.g., stats count() by @timestamp) String fieldName = BinFieldValidator.extractFieldName(node); BinnableField field = new BinnableField(fieldExpr, fieldExpr.getType(), fieldName); diff --git a/docs/user/ppl/cmd/bin.rst b/docs/user/ppl/cmd/bin.rst index 1ebdc3f897..2f7190dd41 100644 --- a/docs/user/ppl/cmd/bin.rst +++ b/docs/user/ppl/cmd/bin.rst @@ -182,6 +182,11 @@ Automatically calculates the span using a mathematical O(1) algorithm to create **Validation**: The bins parameter must be between 2 and 50000 (inclusive). Values outside this range will result in an error. +**Limitation**: The bins parameter on timestamp fields has the following requirements: + +1. **Pushdown must be enabled**: Controlled by ``plugins.calcite.pushdown.enabled`` (enabled by default). When pushdown is disabled, use the ``span`` parameter instead (e.g., ``bin @timestamp span=5m``). +2. **Timestamp field must be used as an aggregation bucket**: The binned timestamp field must be used in a ``stats`` aggregation (e.g., ``source=events | bin @timestamp bins=3 | stats count() by @timestamp``). Using bins on timestamp fields outside of aggregation buckets is not supported. + The algorithm uses **mathematical optimization** instead of iteration for O(1) performance: 1. **Validate bins**: Ensure ``2 ≤ bins ≤ 50000`` diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java index ef8abc3cba..54888369f5 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java @@ -5,6 +5,7 @@ package org.opensearch.sql.calcite.remote; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.opensearch.sql.legacy.TestsConstants.*; import static org.opensearch.sql.util.MatcherUtils.rows; @@ -1126,6 +1127,33 @@ public void testStatsWithBinsOnTimeAndTermField_Avg() throws IOException { rows(40.25, "us-west", "2024-07-01 00:01:00")); } + @Test + public void testBinsOnTimeFieldWithPushdownDisabled_ShouldFail() throws IOException { + // Verify that bins parameter on timestamp fields fails with clear error when pushdown disabled + enabledOnlyWhenPushdownIsDisabled(); + + ResponseException exception = + assertThrows( + ResponseException.class, + () -> + executeQuery( + "source=events_null | bin @timestamp bins=3 | stats count() by @timestamp")); + + // Verify the error message clearly explains the limitation and suggests solutions + // Note: bins parameter on timestamp fields requires BOTH: + // 1. Pushdown to be enabled (plugins.calcite.pushdown.enabled=true, enabled by default) + // 2. The timestamp field to be used as an aggregation bucket (e.g., stats count() by + // @timestamp) + String errorMessage = exception.getMessage(); + assertTrue( + "Expected clear error message about bins parameter requirements on timestamp fields, but" + + " got: " + + errorMessage, + errorMessage.contains("bins' parameter on timestamp fields requires") + && errorMessage.contains("pushdown to be enabled") + && errorMessage.contains("aggregation bucket")); + } + @Test public void testBinWithNestedFieldWithoutExplicitProjection() throws IOException { // Test bin command on nested field without explicit fields projection diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java index 5c2e45f1af..0330d504a9 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java @@ -321,6 +321,10 @@ protected void enabledOnlyWhenPushdownIsEnabled() throws IOException { Assume.assumeTrue("This test is only for when push down is enabled", !isPushdownDisabled()); } + protected void enabledOnlyWhenPushdownIsDisabled() throws IOException { + Assume.assumeTrue("This test is only for when push down is disabled", isPushdownDisabled()); + } + public void updatePushdownSettings() throws IOException { String pushdownEnabled = String.valueOf(GlobalPushdownConfig.enabled); assert !pushdownEnabled.isBlank() : "Pushdown enabled setting cannot be empty";