Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,15 @@ 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 is not supported when pushdown is"
+ " disabled.");
}
throw Util.throwAsRuntime(e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
5 changes: 5 additions & 0 deletions docs/user/ppl/cmd/bin.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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``
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1126,6 +1127,32 @@ 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 not supported on timestamp fields when "
+ "pushdown is disabled, but got: "
+ errorMessage,
errorMessage.contains(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although we enabled push down, we still don't support query like source=events_null | bin @timestamp bins=3.

Shall we call out that it's also required to be used as the aggregation bucket field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comments and documentation

"bins' parameter on timestamp fields is not supported when pushdown is disabled"));
}

@Test
public void testBinWithNestedFieldWithoutExplicitProjection() throws IOException {
// Test bin command on nested field without explicit fields projection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Loading