fix(query): fixed split range query with binary op #1914
Conversation
| * @param plan the plan to compute the offset for. | ||
| */ | ||
| private def computeOffsetMs(plan: LogicalPlan): Seq[Long] = plan match { | ||
| case op: ScalarVectorBinaryOperation => Seq(getOffsetMillis(op).max) |
There was a problem hiding this comment.
Why does this need a special handler for ScalarVerctorBinaryOperations? Was that case missed in getOffsetMillis?
There was a problem hiding this comment.
Scalar has an offset of 0 which can be ignored. Eg.
123 + foo() offset 10m have offset (0m, 10m), we only care about the offset for foo, so we take the max of the two. This gives better efficiency.
(and it would fail lots of old test cases for ScalarVectorBinaryOperation if we don't take max)
There was a problem hiding this comment.
I made it a separate method only because it yells "Cyclomatic complexity of 21 exceeds max of 20." if I don't.
There was a problem hiding this comment.
One query I think might snag here:
foo{} + scalar(foo{} offset 10m)
The complexity to address every case might not be worth the benefit-- how do you feel about ditching this extra logic for now? We can always come back and add a match like this if needed.
There was a problem hiding this comment.
Ditching it is much safer. I'll rewrite/remove the test cases.
| @@ -2401,36 +2401,6 @@ class PlannerHierarchySpec extends AnyFunSpec with Matchers with PlanValidationS | |||
| expected = """E~StitchRvsExec() on InProcessPlanDispatcher(QueryConfig(10 seconds,300000,1,50,antlr,true,true,None,Some(10000),None,None,25,true,false,true,Set(),Some(plannerSelector),Map(filodb-query-exec-metadataexec -> 65536, filodb-query-exec-aggregate-large-container -> 65536),RoutingConfig(false,1800000 milliseconds,true,0))) | |||
| |-E~PromQlRemoteExec(PromQlQueryParams(rate(foo{job="app1"}[5m:30s]) + (rate(bar{job="app1"}[20m:30s]) + count_over_time(baz{job="app1"}[5m:30s])),0,3,5000,None,false), PlannerParams(filodb,None,None,None,None,60000,PerQueryLimits(1000000,18000000,100000,100000,300000000,1000000,200000000),PerQueryLimits(50000,15000000,50000,50000,150000000,500000,100000000),None,None,None,false,86400000,86400000,false,true,false,false,true,10,false,true,TreeSet(),LegacyFailoverMode,None,None,None,None), queryEndpoint=remote0-url, requestTimeoutMs=10000) on InProcessPlanDispatcher(QueryConfig(10 seconds,300000,1,50,antlr,true,true,None,Some(10000),None,None,25,true,false,true,Set(),Some(plannerSelector),Map(filodb-query-exec-metadataexec -> 65536, filodb-query-exec-aggregate-large-container -> 65536),RoutingConfig(false,1800000 milliseconds,true,0))) | |||
| |-E~PromQlRemoteExec(PromQlQueryParams(rate(foo{job="app1"}[5m:30s]) + (rate(bar{job="app1"}[20m:30s]) + count_over_time(baz{job="app1"}[5m:30s])),6501,3,9999,None,false), PlannerParams(filodb,None,None,None,None,60000,PerQueryLimits(1000000,18000000,100000,100000,300000000,1000000,200000000),PerQueryLimits(50000,15000000,50000,50000,150000000,500000,100000000),None,None,None,false,86400000,86400000,false,true,false,false,true,10,false,true,TreeSet(),LegacyFailoverMode,None,None,None,None), queryEndpoint=remote1-url, requestTimeoutMs=10000) on InProcessPlanDispatcher(QueryConfig(10 seconds,300000,1,50,antlr,true,true,None,Some(10000),None,None,25,true,false,true,Set(),Some(plannerSelector),Map(filodb-query-exec-metadataexec -> 65536, filodb-query-exec-aggregate-large-container -> 65536),RoutingConfig(false,1800000 milliseconds,true,0)))""".stripMargin), | |||
| Test("""test{job="app"} + 123""", | |||
There was a problem hiding this comment.
Confirming: all tests are either added or moved? None are deleted?
There was a problem hiding this comment.
These are the identical test cases except these run on a partition has gRPC QS. I kept most of them and moved some under "split-partition queries with lookback and offset "
Co-authored-by: Brian-Yu <[email protected]>
Co-authored-by: Brian-Yu <[email protected]>
Pull Request checklist
Current behavior :
After namespace migration, range query that cross the migration/split point give wrong results when lhs and rhs have different offsets in a binary operation.
New behavior :
It gives the correct result.