-
Notifications
You must be signed in to change notification settings - Fork 170
Support time modifiers in search command #4224
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?
Conversation
can you review this: #4152 |
Hi @vamsimanohar, thank you for reminding! I think the functionality does not overlap, I'll re-implement this based on your grammar. |
@yuancu Few things to keep in mind.
I think it should be possible with your changes. |
Signed-off-by: Yuanchun Shen <[email protected]> Unit test search with absolute time range Signed-off-by: Yuanchun Shen <[email protected]> Rephrase timeRange and timeModifier Signed-off-by: Yuanchun Shen <[email protected]> Switch to earliest and latest udf Signed-off-by: Yuanchun Shen <[email protected]> Add a convert util Signed-off-by: Yuanchun Shen <[email protected]> Verify time correctness during coversion Signed-off-by: Yuanchun Shen <[email protected]> Fix quarter parsing bugs Signed-off-by: Yuanchun Shen <[email protected]> Fix week snap parsing Signed-off-by: Yuanchun Shen <[email protected]> Remove old implementation that translates time modifier to time filter Signed-off-by: Yuanchun Shen <[email protected]>
getRelativeZonedDateTime( | ||
expression, ZonedDateTime.ofInstant(clock.instant(), clock.getZone())); | ||
return earliest.isBefore(candidateDatetime); | ||
return !earliest.isAfter(candidateDatetime); |
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.
Modified because earliest
should be greater or equal.
Signed-off-by: Yuanchun Shen <[email protected]>
Signed-off-by: Yuanchun Shen <[email protected]>
Signed-off-by: Yuanchun Shen <[email protected]>
; | ||
|
||
timeSnap | ||
: AT timeModifierUnit; |
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.
Asking for help: currently, AT timeModifierUnit
can not be matched. Since antlr is parsed bottom-up, plus that a shorter rule matching longer text will be prioritized, AT timeModifierUnit
will always be matched as an identifier by the following rule:
// OpenSearchPPLLexer.g4
ID: ID_LITERAL;
fragment ID_LITERAL: ([@*A-Z_])+?[*A-Z_\-0-9]*;
// OpenSearchPPLParser.g4
ident
: (DOT)? ID
As a result, I can not write something like source=t earliest=-10h@day
, but have to write it as source=t earliest='-10h@day'
because -10h@day
will be matched to the shorter rule ident
.
I tried to make the grammar context aware, but had no luck with multiple solutions. Is there any suggestion for a workaround?
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.
@yuancu have you figured out anything on this?
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.
No, I haven't :/
testImplementation group: 'org.hamcrest', name: 'hamcrest-library', version: "${hamcrest_version}" | ||
testImplementation group: 'org.mockito', name: 'mockito-core', version: "${mockito_version}" | ||
testImplementation group: 'org.mockito', name: 'mockito-junit-jupiter', version: "${mockito_version}" | ||
testImplementation group: 'org.opensearch', name: 'opensearch', version: "${opensearch_version}" |
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.
This dependency is added for the access of org.opensearch.common.time.DateMathParser
-- I want to make sure that the parsed OpenSearch date math like now-1d/M-2M
returns the intended instant for -1d@q
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.
parseRelativeTime() is called by PPL module only, right? if move parseRelativeTime to PPL module, does opensearch depedency still needed?
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.
Yes, it is only called by PPL module. If I still want to assure the correctness of the parsed instant, I'll have to move this dependency to the PPL module.
Is there any concern over this dependency? If it's for the performance, as the dependency is only test time, I assume there would be no harm to the runtime performance.
Signed-off-by: Yuanchun Shen <[email protected]>
@yuancu hey can you update the description with latest implementation where you are translating to Lucene query. |
HRS: 'HRS'; | ||
HOURS: 'HOURS'; | ||
DAYS: 'DAYS'; | ||
WEEKS: 'WEEKS'; |
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.
can you add these new terms to searchKeyWords or keywordCanbeId
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.
Added timeModifierUnit
to searchableKeyWord
. I wonder how does this change the parsing behavior
|
||
searchExpression | ||
: LT_PRTHS searchExpression RT_PRTHS # groupedExpression | ||
: timeModifier # timeModifierExpression |
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.
For my understanding: This implies we can write only timeModifier in searchExpression.
Can you update search.rst with these changes.
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.
+1, please update doc.
JSONObject result1 = | ||
executeQuery( | ||
String.format( | ||
"search source=%s earliest='2025-08-01 03:47:41' latest=now | fields @timestamp", |
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 if earliest is a field name? Should add backtick?
|
||
searchExpression | ||
: LT_PRTHS searchExpression RT_PRTHS # groupedExpression | ||
: timeModifier # timeModifierExpression |
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.
+1, please update doc.
: LT_PRTHS searchExpression RT_PRTHS # groupedExpression | ||
: timeModifier # timeModifierExpression | ||
| LT_PRTHS searchExpression RT_PRTHS # groupedExpression | ||
| NOT searchExpression # notExpression |
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.
do timemodifier support NOT? e.g. search source=index NOT latest=now
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.
Yes. Time modifiers are converted conditions with SearchComparison
. Any further operation that is applicable to search comparison, such as NOT, OR, AND, are applicable to time modifiers.
testImplementation group: 'org.hamcrest', name: 'hamcrest-library', version: "${hamcrest_version}" | ||
testImplementation group: 'org.mockito', name: 'mockito-core', version: "${mockito_version}" | ||
testImplementation group: 'org.mockito', name: 'mockito-junit-jupiter', version: "${mockito_version}" | ||
testImplementation group: 'org.opensearch', name: 'opensearch', version: "${opensearch_version}" |
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.
parseRelativeTime() is called by PPL module only, right? if move parseRelativeTime to PPL module, does opensearch depedency still needed?
Description
Support time modifier in search command.
Changes are cherry-picked from a branch based on #4152. Waiting for #4152 to be completed.Examples:
Queries with earliest and latest time modifiers will be converted to a query string query with comparison conditions on the implicit timestamp field
@timestamp
. For example, querysource=time_test earliest=-1year latest='-50d@w3'
is converted to the following DSL:Work items
2012-10-10
now
,-30m
,-2h@h
,@w0
-mon@mon+7d
1
(UTC January 1, 1970 at 12:00:01 AM)Related Issues
Resolves #4135
Implementation Walk-through
Implementation Details
The core functionality is implemented in the
visitTimeModifierValue
andvisitTimeModifierExpression
methods in AstExpressionBuilder.java:visitTimeModifierValue
: Converts time values to OpenSearch date math expressionsvisitTimeModifierExpression
: Creates comparison conditions for@timestamp
field in query stringThe PR extends DateTimeUtils.java with the parseRelativeTime method, which transforms PPL time expressions to OpenSearch date math expressions.
Time modifiers are converted to search comparisons with a query string on the implicit
@timestamp
field, which is now defined as a constant in OpenSearchConstants.java. For example,search earliest=-10days@month latest=now()
is converted to a query string query like the following:PPL Time Modifiers to OpenSearch Date Math Examples
Absolute Times
Relative Times
Snapping to Time Units (Using @)
Week Days
Special Cases
Check List
--signoff
or-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.