-
Notifications
You must be signed in to change notification settings - Fork 10
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
ENG-46252 : Pinot time range filtering support #224
Conversation
# additionalTenantFilters = [ | ||
# { | ||
# "tenantId" : "testTenant", | ||
# "timeRangeAndFilters" : [ |
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 does multiple timeRangeAndFilters
mean here?
So, different filters for multiple time ranges, right?
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
# "timeRangeAndFilters" : [ | ||
# { | ||
# "start_time_millis" : 1, | ||
# "endTimeMillis" : 1, |
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.
endTimeMillis
-> end_time_millis
This will be an epoch time, right?
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, but I'm thinking of making start_time_millis
as starTimeMillis to keep it consistent with the other keys
@@ -82,6 +82,18 @@ service.config = { | |||
requestHandlerInfo = { | |||
tenantColumnName = tenant_id | |||
startTimeAttributeName = "EVENT.startTime" | |||
# additionalTenantFilters = [ |
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.
additionalTenantFilters
-> scopedAdditionalFilters
?
Just a thought: Do you think we might need to extend that in the future?
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.
since it's already under a table scope, I think we not need add another scope here.
I don't think we need to further extend it to query scope(Its not even available in query as we discussed)
try { | ||
JsonNode jsonNode = objectMapper.readTree(filterJson); | ||
|
||
Filter.Builder personBuilder = Filter.newBuilder(); |
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.
personBuilder
-> fitlerBuilder
@@ -3,9 +3,6 @@ | |||
import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_COUNT; | |||
import static org.hypertrace.core.query.service.QueryRequestUtil.getAlias; | |||
import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnName; | |||
import static org.hypertrace.core.query.service.api.Expression.ValueCase.ATTRIBUTE_EXPRESSION; |
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.
Did we move this some other place?
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.
reverted
Can we add some tests? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
{ | ||
"startTimeMillis" = 1 | ||
"endTimeMillis" = 2 | ||
"filter" : "{\r\n \"lhs\": {\r\n \"columnIdentifier\": {\r\n \"columnName\": \"SERVICE.name\"\r\n }\r\n },\r\n \"operator\": \"EQ\",\r\n \"rhs\": {\r\n \"literal\": {\r\n \"value\": {\r\n \"string\": \"dummyService\"\r\n }\r\n }\r\n }\r\n}" |
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.
Q: Do we need \r\n
? If we just put a valid single line json string, would it work?
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.
Yeah, it would work. Was just added by the json string escaper I was using, got rid of it.
"tenantId" = "__default" | ||
"timeRangeAndFilters" = [ | ||
{ | ||
"filter" : "{\r\n \"lhs\": {\r\n \"columnIdentifier\": {\r\n \"columnName\": \"SERVICE.name\"\r\n }\r\n },\r\n \"operator\": \"EQ\",\r\n \"rhs\": {\r\n \"literal\": {\r\n \"value\": {\r\n \"string\": \"dummyService\"\r\n }\r\n }\r\n }\r\n}" |
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 we need \r\n
? If we just put a single string, would it work?
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.
removed
} | ||
} | ||
|
||
private Optional<Filter> buildTimeRangeAndFilters(Optional<String> timeRangeAttributeOptional) { |
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.
nit: timeRangeAttributeOptional -> timeRangeAttribute
config.getString("name"), config.getConfig("requestHandlerInfo")); | ||
|
||
// domain-service-handler only has filter and no time range | ||
if (config.getString("name").equals("domain-service-handler")) { |
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.
Is this test using the correct handler - domain-service-handler
? For this, the original request should be the same after applying filters, right?
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, it should have been - trace-view-handler
, right?
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.
domain-event-view only has filter.
span-event-view only has time range
trace-view has both time range and filter.
if (this.startTimeMillis.isPresent() && this.endTimeMillis.isPresent()) { | ||
filterBuilder.addChildFilter( | ||
QueryRequestUtil.createLongFilter( | ||
timeRangeAttributeOptional.get(), Operator.GT, this.endTimeMillis.get())); |
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.
Q: Is this check correct? Should endTime
be compared with the greater than operator (I don't remember the text you had in your sublime, so just re-checking)?
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 this is how it's supposed to be as it's an or filter
This is what we discussed.
t3.... t1 t2 ....t4
((s >= t2 OR s <= t1) OR (API NOT IN (api1, api2)) AND (t3, t4))
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -27,4 +27,9 @@ public interface RequestHandler { | |||
|
|||
/** Handle the request and add rows to the collector. */ | |||
Observable<Row> handleRequest(QueryRequest request, ExecutionContext executionContext); | |||
|
|||
default QueryRequest applyAdditionalFilters( |
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.
There is already an existing step of query transformation. Look at QueryTransformation
. This step of adding filters should go there.
https://github.com/hypertrace/query-service/blob/main/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTransformation.java
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.
We started from this, then realised we don't get scope information in query request.
If we apply a filter on Transformation step, it'll be applied on all the scopes and would work as global filter. Which is wrong behaviour
Query Service decides the scope and it's handler based of the query. Hence pushed down these filters to handler
This comment has been minimized.
This comment has been minimized.
...ice-impl/src/main/java/org/hypertrace/core/query/service/AdditionalHandlerFiltersConfig.java
Outdated
Show resolved
Hide resolved
query-service/src/main/resources/configs/common/application.conf
Outdated
Show resolved
Hide resolved
|
||
private Optional<Filter> buildTimeRangeAndFilters(Optional<String> timeRangeAttribute) { | ||
Filter.Builder filterBuilder = Filter.newBuilder(); | ||
filterBuilder.setOperator(Operator.OR); |
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.
why OR? don't we want the filter to be: time > startTime and time < endTime?
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.
...ice-impl/src/main/java/org/hypertrace/core/query/service/AdditionalHandlerFiltersConfig.java
Outdated
Show resolved
Hide resolved
...ice-impl/src/main/java/org/hypertrace/core/query/service/AdditionalHandlerFiltersConfig.java
Outdated
Show resolved
Hide resolved
...ice-impl/src/main/java/org/hypertrace/core/query/service/AdditionalHandlerFiltersConfig.java
Outdated
Show resolved
Hide resolved
@@ -39,6 +39,18 @@ service.config = { | |||
startTimeAttributeName = "Trace.start_time_millis" | |||
slowQueryThresholdMs = 0 | |||
percentileAggFunction = "PERCENTILE" | |||
additionalTenantFilters = [ |
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.
There are multiple problems in the way this new config has been defined:
- It is is very confusing. Only after going through the code one can understand.
- There is a hardcoded assumption that
startTimeAttributeName
will act as the lhs key for the 2 values below. Makes it very specific and no room for generalising. - It is hardcoded with
OR
operator.
I would prefer it being defined as follows:
additionalHandlerConfigs = [
operator = "OR"
{
key = "Trace.start_time_millis"
operator = LT
value = "max_cutoff"
},
{
key = "Trace.start_time_millis"
operator = GE
value = "min_cutoff"
},
filter : {
columnIdentifiier: ...
}
]
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.
Would operator = "AND"
possible here?
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.
We would like to apply the time range to startTimeAttributeName
. Any other attribute can go to the normal filter expression.
The given time range indicates that we would like to apply a filter within that specified range. is that not clear?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
lgtm
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
LGTM
No description provided.