-
Notifications
You must be signed in to change notification settings - Fork 180
Add per_second function support for timechart command
#4464
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
Add per_second function support for timechart command
#4464
Conversation
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
RyanL1997
left a comment
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.
Hi @dai-chen , thanks for the change.
|
|
||
| return eval( | ||
| timechart(AstDSL.alias(perFunc.aggName, sum(perFunc.aggArg))), | ||
| let(perFunc.aggName).multiply(perFunc.seconds).dividedBy(spanSeconds)); |
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.
What happens if the span evaluates to 0 seconds (e.g., with millisecond spans or edge cases)? Should there be validation or error handling for division by zero?
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.
Good catch. Let me check if timechart has the validation or not. Thanks!
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 quick tested timechart command and found negative span value is not allowed in our grammar. But zero-span validation is missing. Let me create an issue and raise separate PR to fix.
opensearchsql> source=test_data_2023 | timechart span=0m per_second(packets);
TransportError(500, 'SearchPhaseExecutionException', {'error':
{'reason':'Error occurred in OpenSearch engine: all shards failed', 'details': 'Shard[0]:
java.lang.IllegalArgumentException: Zero or negative time interval not supported\n\n
For more details, please send request for Json format to see the raw response from OpenSearch engine.',
'type': 'SearchPhaseExecutionException'}, 'status': 400})
opensearchsql> source=test_data_2023 | timechart span=-1m per_second(packets);
{'reason': 'Invalid Query', 'details': "[-] is not a valid term at this part of the query: '...23
| timechart span=-' <-- HERE. extraneous input '-' expecting {SPANLENGTH, INTEGER_LITERAL}",
'type': 'SyntaxCheckException'}
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.
Created issue: #4527. Thanks!
core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTimechartPerFunctionIT.java
Show resolved
Hide resolved
Signed-off-by: Chen Dai <[email protected]>
Add per_second() support to the timechart command by implementing Option 3 (Eval Transformation). --------- Signed-off-by: Chen Dai <[email protected]> (cherry picked from commit 4d416db) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
) Add per_second() support to the timechart command by implementing Option 3 (Eval Transformation). --------- (cherry picked from commit 4d416db) Signed-off-by: Chen Dai <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Lantao Jin <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Lantao Jin <[email protected]>
Description
This PR adds
per_second()support to thetimechartcommand by implementing Option 3 (Eval Transformation). As a short-term solution, it extends theTimechartAST to recognize the per_second function and rewrites into the equivalent math formula shown in the examples below. See issue #4350 for background and alternatives.Examples
For spans whose length varies with the calendar (month/quarter/year), the number of seconds depends on actual start/end timestamps (e.g., September has 30 days; October has 31 days; February in a leap year has 29 days). To ensure correctness, the rewrite computes the exact bucket length dynamically:
-- Example: span=2mon source=logs | timechart span=2mon sum(packets) as "per_second(packets)" | eval `per_second(packets)` = `per_second(packets)` / timestampdiff( SECOND, @timestamp, -- span start timestampadd(MONTH, 2, @timestamp) -- span end (start + 2 months) )Related Issues
Resolves partially #4350.
Check List
--signoffor-s.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.