feat(query) Implement @modifier. #1678

yu-shipit · 2023-09-29T23:13:54Z

Override range param based on @modifier timestamp for PeriodicSamplesMapper.
Add RepeatTransformer and RepeatValueVector to populate data for all timestamps.
Add GRPC functions for RepeatValueVector.

Pull Request checklist

The commit(s) message(s) follows the contribution guidelines ?
Tests for the changes have been added (for bug fixes / features) ?
Docs have been added / updated (for bug fixes / features) ?

Current behavior : (link exiting issues here : https://help.github.com/articles/basic-writing-and-formatting-syntax/#referencing-issues-and-pull-requests)

New behavior :
Will support @modifier.
The problem is described in this doc.
The prometheus PR.
Filodb will support advance usage such as

rate(process_cpu_seconds_total[1m]) 
  and
topk(7, rate(process_cpu_seconds_total[1h] @ 1234))

Other information:

yu-shipit · 2023-09-29T23:28:31Z

credits to @alextheimer PR.
#1281

alextheimer

Thanks for the PR @yu-shipit -- just a couple of questions. Also, please add some tests that show the ExecPlans for simple queries and typical @ use-cases.

coordinator/src/main/scala/filodb.coordinator/queryplanner/DefaultPlanner.scala

@modifier

1. Override range param based on @modifier timestamp for PeriodicSamplesMapper. 2. Add RepeatTransformer and RepeatValueVector to populate data for all timestamps. 3. Add GRPC functions for RepeatValueVector.

coordinator/src/main/scala/filodb.coordinator/queryplanner/LongTimeRangePlanner.scala

alextheimer · 2023-10-30T20:52:45Z

query/src/main/scala/filodb/query/LogicalPlan.scala

+    atMs.forall(at => (at >= earliestRawTimeWithOffset && startMs >= earliestRawTimeWithOffset)
+      || (at < earliestRawTimeWithOffset && endMs < earliestRawTimeWithOffset))


startMs and endMs don't necessarily correlate with the range of data a query will read. Consider the instant query:

rate(foo{...}[14d]) // startMs=endMs

coordinator/src/main/scala/filodb.coordinator/queryplanner/LongTimeRangePlanner.scala

alextheimer · 2023-10-30T22:15:26Z

coordinator/src/test/scala/filodb.coordinator/queryplanner/PlannerHierarchySpec.scala

@@ -1837,47 +1837,67 @@ class PlannerHierarchySpec extends AnyFunSpec with Matchers with PlanValidationS
    validatePlan(execPlan, expectedPlan)
  }

-  it("Modifier should have PeriodicSamplesMapper wrapped by RepeatTransformer") {
+  it("Modifier should have PeriodicSamplesMapper wrapped by RepeatTransformer and all data in raw cluster") {


Could you add a test case for each possible scenario?

query routed entirely to raw (already exists)

query routed entirely to DS (already exists)

query routed to both (should reject)

query start/end are in raw, but lookback extends into DS (should reject)

query start/end are in raw, but offset pushes it entirely into DS (ok)

[any other important scenarios I'm forgetting]

Thanks for all these tests 🙏

coordinator/src/main/scala/filodb.coordinator/queryplanner/LongTimeRangePlanner.scala

alextheimer · 2023-11-07T06:54:25Z

coordinator/src/test/scala/filodb.coordinator/queryplanner/PlannerHierarchySpec.scala

@@ -1837,47 +1837,67 @@ class PlannerHierarchySpec extends AnyFunSpec with Matchers with PlanValidationS
    validatePlan(execPlan, expectedPlan)
  }

-  it("Modifier should have PeriodicSamplesMapper wrapped by RepeatTransformer") {
+  it("Modifier should have PeriodicSamplesMapper wrapped by RepeatTransformer and all data in raw cluster") {


Thanks for all these tests 🙏

query/src/main/scala/filodb/query/LogicalPlan.scala

coordinator/src/main/scala/filodb.coordinator/queryplanner/LongTimeRangePlanner.scala

This reverts commit 68298f2.

friendly messages.

query/src/main/scala/filodb/query/LogicalPlan.scala

coordinator/src/main/scala/filodb.coordinator/queryplanner/LongTimeRangePlanner.scala

alextheimer · 2023-11-10T17:49:59Z

coordinator/src/main/scala/filodb.coordinator/queryplanner/DefaultPlanner.scala

+     val realScanStartMs = sqww.atMs.getOrElse(sqww.startMs)
+     val realScanEndMs = sqww.atMs.getOrElse(sqww.endMs)
+     val realScanStep = sqww.atMs.map(_ => 0L).getOrElse(sqww.stepMs)


Should these instead be something like this?:

val realScanStartMs = sqww.atMs.map(at => at - (sqww.endMs - sqww.startMs)).getOrElse(sqww.startMs) val realScanEndMs = sqww.atMs.getOrElse(sqww.endMs) // realScanStep remains unchanged

Somehow an instant query requires 0 to be step .

alextheimer · 2023-11-10T17:57:08Z

coordinator/src/main/scala/filodb.coordinator/queryplanner/LongTimeRangePlanner.scala

+      atModifierTimestampsWithOffset.isEmpty
+    }
+    require(isAtModifierValid, s"@modifier $atModifierTimestampsWithOffset is not supported. Because it queries" +
+      s"both down sampled and raw data. Please adjust the start and end time if you want to use @modifier.")


Should this instead say "please adjust query parameters"? (An instant query might lookback too far.)

alextheimer · 2023-11-10T18:01:01Z

coordinator/src/main/scala/filodb.coordinator/queryplanner/SingleClusterPlanner.scala

+        case w: PeriodicSeriesWithWindowing => (w.startMs, w.stepMs, w.window, w.offsetMs.getOrElse(0L),
+          w.atMs.getOrElse(0L), w.endMs)


Double-checking: no need to update these start/step/end values like we do for subqueries?

alextheimer · 2023-11-10T18:05:26Z

coordinator/src/main/scala/filodb.coordinator/queryplanner/SingleClusterPlanner.scala

-          Some(LogicalPlanUtils.copyLogicalPlanWithUpdatedTimeRange(logicalPlan, TimeRange(newStartMs, boundParams._5)))
+          Some(LogicalPlanUtils.copyLogicalPlanWithUpdatedTimeRange(logicalPlan, TimeRange(newStartMs, boundParams._6)))


Outside the scope of this PR, but it's probably safe to say it's time for those boundParams to be given their own case class 😆

Yeah. we have one more argument, which change the number to 6.

alextheimer · 2023-11-10T18:06:42Z

coordinator/src/main/scala/filodb.coordinator/queryplanner/SingleClusterPlanner.scala

+    val realScanStartMs = lp.atMs.getOrElse(logicalPlanWithoutBucket.startMs)
+    val realScanEndMs = lp.atMs.getOrElse(logicalPlanWithoutBucket.endMs)
+    val realScanStepMs: Long = lp.atMs.map(_ => 0L).getOrElse(lp.stepMs)


Same as some other comments: should we keep the original duration between start/end?

alextheimer · 2023-11-10T18:10:22Z

core/src/main/scala/filodb.core/query/RangeVector.scala

+  override def rows(): RangeVectorCursor = {
+    import NoCloseCursor._
+    // If rowReader is empty, iterate nothing.
+    val it = Iterator.from(0, rowReader.map(_ => stepMs.toInt).getOrElse(0))


Should this be getOrElse(1) to prevent an infinite loop?

query/src/main/scala/filodb/query/LogicalPlan.scala

yu-shipit requested review from amolnayak311, vishramachandran, sherali42 and alextheimer September 29, 2023 23:13

yu-shipit force-pushed the modifier branch from 30b949a to c42f380 Compare September 29, 2023 23:19

yu-shipit force-pushed the modifier branch from c42f380 to 15f6bec Compare October 2, 2023 21:20

alextheimer reviewed Oct 6, 2023

View reviewed changes

yu-shipit requested a review from alextheimer October 10, 2023 01:42

yu-shipit force-pushed the modifier branch 3 times, most recently from d9786b5 to 1dfc95a Compare October 13, 2023 17:11

Yu Zhang added 4 commits October 17, 2023 16:55

feat(query) Implement @modifier.

3e0bd1b

1. Override range param based on @modifier timestamp for PeriodicSamplesMapper. 2. Add RepeatTransformer and RepeatValueVector to populate data for all timestamps. 3. Add GRPC functions for RepeatValueVector.

add proto buf test for RepeatRangeVector.

835ec3e

rename the confusing variables.

78c18a9

add test to PlannerHierarchySpec.

a02b584

yu-shipit force-pushed the modifier branch from 20da410 to a02b584 Compare October 17, 2023 23:55

add sanity check for @modifier.

c5ee842

alextheimer requested changes Oct 30, 2023

View reviewed changes

yu-shipit force-pushed the modifier branch from e6540b4 to 1842bcc Compare November 6, 2023 19:52

handle offset and add more test cases.

68298f2

yu-shipit force-pushed the modifier branch from 1842bcc to 68298f2 Compare November 6, 2023 21:43

alextheimer reviewed Nov 7, 2023

View reviewed changes

Yu Zhang added 4 commits November 7, 2023 08:18

Revert "handle offset and add more test cases."

a921475

This reverts commit 68298f2.

fix the sanity check

86d0e56

fix sanity check.

ff2624c

adjust @modifier with offset.

7574b0e

friendly messages.

yu-shipit force-pushed the modifier branch from 2b82323 to 7574b0e Compare November 7, 2023 22:56

alextheimer reviewed Nov 8, 2023

View reviewed changes

query/src/main/scala/filodb/query/LogicalPlan.scala Outdated Show resolved Hide resolved

coordinator/src/main/scala/filodb.coordinator/queryplanner/LongTimeRangePlanner.scala Outdated Show resolved Hide resolved

yu-shipit requested a review from alextheimer November 9, 2023 00:55

yu-shipit force-pushed the modifier branch 2 times, most recently from 8510f58 to 48670b2 Compare November 9, 2023 18:45

alextheimer reviewed Nov 10, 2023

View reviewed changes

fix the message.

041050e

yu-shipit force-pushed the modifier branch from 48670b2 to 041050e Compare November 14, 2023 01:31

Yu Zhang added 2 commits November 15, 2023 14:29

refact the code.

ff25c34

explicitly add all cases.

678efb2

alextheimer approved these changes Nov 17, 2023

View reviewed changes

Merge branch 'filodb:develop' into modifier

0ef5b61

yu-shipit merged commit 0e49dc7 into filodb:develop Nov 17, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(query) Implement @modifier. #1678

feat(query) Implement @modifier. #1678

yu-shipit commented Sep 29, 2023 •

edited

Loading

yu-shipit commented Sep 29, 2023 •

edited

Loading

alextheimer left a comment

alextheimer Oct 30, 2023

alextheimer Oct 30, 2023

alextheimer Nov 7, 2023

alextheimer Nov 7, 2023

alextheimer Nov 10, 2023

yu-shipit Nov 11, 2023

alextheimer Nov 10, 2023

alextheimer Nov 10, 2023

alextheimer Nov 10, 2023

yu-shipit Nov 10, 2023

alextheimer Nov 10, 2023

alextheimer Nov 10, 2023

		atMs.forall(at => (at >= earliestRawTimeWithOffset && startMs >= earliestRawTimeWithOffset)
		\|\| (at < earliestRawTimeWithOffset && endMs < earliestRawTimeWithOffset))

		case w: PeriodicSeriesWithWindowing => (w.startMs, w.stepMs, w.window, w.offsetMs.getOrElse(0L),
		w.atMs.getOrElse(0L), w.endMs)

		Some(LogicalPlanUtils.copyLogicalPlanWithUpdatedTimeRange(logicalPlan, TimeRange(newStartMs, boundParams._5)))
		Some(LogicalPlanUtils.copyLogicalPlanWithUpdatedTimeRange(logicalPlan, TimeRange(newStartMs, boundParams._6)))

feat(query) Implement @modifier. #1678

feat(query) Implement @modifier. #1678

Conversation

yu-shipit commented Sep 29, 2023 • edited Loading

yu-shipit commented Sep 29, 2023 • edited Loading

alextheimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yu-shipit commented Sep 29, 2023 •

edited

Loading

yu-shipit commented Sep 29, 2023 •

edited

Loading