-
Notifications
You must be signed in to change notification settings - Fork 178
Enhance doc and error message handling for bins on time-related fields
#4713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
e30cceb
d2fb116
92e4109
2aa1768
bc510e7
6a17894
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,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( | ||
|
||
| "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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change error message as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated error message