Support millisecond span#4672
Conversation
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
| function( | ||
| "timestampdiff", | ||
| stringLiteral("SECOND"), | ||
| stringLiteral("MILLISECOND"), |
There was a problem hiding this comment.
Can you add some tests for MICROSECOND
| Content-Type: 'application/json' | ||
| ppl: | ||
| body: | ||
| query: source=test_data_2023 | timechart span=500ms count() |
There was a problem hiding this comment.
can you add a test for MICROSECOND:
source=test_data_2023 | timechart span=500000us count()
There was a problem hiding this comment.
It seems the minimum date time unit supportted in span function is millisecond:
- SpanFunction.java
public static Object evalTimestamp( @Parameter(name = "value") String value, @Parameter(name = "interval") int interval, @Parameter(name = "unit") String unit) { ExprValue exprInterval = ExprValueUtils.fromObjectValue(interval, ExprCoreType.INTEGER); ExprValue exprValue = ExprValueUtils.fromObjectValue(value, ExprCoreType.TIMESTAMP); Rounding<?> rounding = new TimestampRounding(exprInterval, unit); return rounding.round(exprValue).valueForCalcite(); }
- then in Rounding.java
DateTimeUnit.resolve(unit) ... public enum DateTimeUnit { @Getter private final int id; @Getter private final String name; protected final boolean isMillisBased; protected final long ratio; MILLISECOND(1, "ms", true, ChronoField.MILLI_OF_SECOND.getBaseUnit().getDuration().toMillis()) { @Override public long round(long utcMillis, int interval) { return DateTimeUtils.roundFloor(utcMillis, ratio * interval); } }, SECOND(2, "s", true, ChronoField.SECOND_OF_MINUTE.getBaseUnit().getDuration().toMillis()) { @Override public long round(long utcMillis, int interval) { return DateTimeUtils.roundFloor(utcMillis, ratio * interval); } }, ... }
DateTimeUnit is built based on milliseconds. Its ration attribute if of type long, thus can not represent a fraction of millisecond.
Should I refactor this part to extend its support to microsecond and nanosecond?
There was a problem hiding this comment.
What does the current user doc say?
There was a problem hiding this comment.
In the doc for stats command, it claims that up to milliseconds is supported:
| Span Interval Units |
|---|
| millisecond (ms) |
| second (s) |
| minute (m, case sensitive) |
| hour (h) |
| day (d) |
| week (w) |
| month (M, case sensitive) |
| quarter (q) |
| year (y) |
There was a problem hiding this comment.
Ok, we can file a new PR to support MICROSECOND , as well as source=test_data_2023 | timechart span=500000us count()
| SPANLENGTH: [0-9]+ ( | ||
| 'US'|'MS'|'CS'|'DS' | ||
| 'US' |'CS'|'DS' | ||
| |'MS'|'MILLISECOND'|'MILLISECONDS' |
There was a problem hiding this comment.
If we want to expose microsecond span support, we need to update all docs related? Also we have to change per_* function to microsecond-based, otherwise timechart span=1us per_second... will break?
I'm thinking shall we separate the PR, e.g., fix mismatch of microsecond and millisecond in this PR and decide whether to expose all subsecond to PPL span clause later?
There was a problem hiding this comment.
It seems microseconds are not supported by span yet.
I'm wondering if we are going to expand the support to microseconds, as nanoseconds are also supported in PPL, should we also do it for nanoseconds?
There was a problem hiding this comment.
I'm under the same impression for span with timechart command. Probably we can track this as separate task and don't need to make more changes for span and per_* function here.
| UNKNOWN("unknown"), | ||
| NONE(""), | ||
| MICROSECOND("us"), | ||
| US("us"), |
There was a problem hiding this comment.
question: μs? Why fear unicode in the strings?
There was a problem hiding this comment.
That's a good point. I didn't think of it. I used us because it was already used to represent microseconds in the grammar file:
sql/ppl/src/main/antlr/OpenSearchPPLLexer.g4
Line 505 in fcff083
Besides, us has the merit in its convenience of typing; SPL also adopts us for microseconds:
Error in 'timechart' command: The value for option span (1μs) is invalid. When span is expressed using a sub-second unit (ds, cs, ms, us), the span value needs to be < 1 second, and 1 second must be evenly divisible by the span value.
Therefore, I think it's acceptable to adopt us for our use case. But of course, we can extend the support to μs.
There was a problem hiding this comment.
It comes to me that cs and ds are not supported well in span as well. I'll raise another issue for it.
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-4672-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b224750d9bb865aa6695055c6ec4246485f597c2
# Push it to GitHub
git push --set-upstream origin backport/backport-4672-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
* Support millisecond span Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Update per funciton tests Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> --------- Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> (cherry picked from commit b224750)
* Support millisecond span Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Update per funciton tests Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> --------- Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Description
Milliseconds had been improperly converted to microseconds. This PR fixes the issue by incorporating support to milliseconds.
Additionally, this PR correct a minor flaw of per function implementations so that they work properly with milliseconds.
Related Issues
Resolves #4550
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.