Skip to content
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

feat: improve watermark calculation by adding the option to cache it #6413

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

AdityaHegde
Copy link
Collaborator

@AdityaHegde AdityaHegde commented Jan 14, 2025

This PR centralises the calculation of min, max and watermark to executors.

  • Adds Executor.Timestamps that does the calculation of min, max (previously in metrics_time_range resolver) and watermark.
  • Updates metrics_time_range resolver to call Executor.Timestamps.
  • Adds Executor.BindQuery that allows for setting the timestamps from outside. This allows for getting a cached timestamps by getting it from metrics_time_range resolver.
  • Update MetricsViewTimeRange to use metrics_time_range resolver.
  • Update usages of Executor.Query to set cached timestamps.

A future PR will refactor metrics_sql to move time range resolution to Executor. So it is not updated to use the new resolver.

@AdityaHegde AdityaHegde marked this pull request as ready for review January 14, 2025 15:28
@AdityaHegde AdityaHegde force-pushed the feat/watermark-calc-to-resolver branch from abab4ec to 0cc9827 Compare January 14, 2025 15:46
proto/rill/runtime/v1/queries.proto Outdated Show resolved Hide resolved
runtime/metricsview/executor.go Outdated Show resolved Hide resolved
runtime/metricsview/executor.go Outdated Show resolved Hide resolved
runtime/metricsview/executor_timestamps.go Outdated Show resolved Hide resolved
runtime/metricsview/executor_timestamps.go Outdated Show resolved Hide resolved
runtime/queries/metricsview_aggregation.go Outdated Show resolved Hide resolved
runtime/queries/metricsview_aggregation.go Outdated Show resolved Hide resolved
runtime/queries/metricsview_aggregation.go Outdated Show resolved Hide resolved
runtime/queries/metricsview_aggregation.go Outdated Show resolved Hide resolved
runtime/queries/metricsview_aggregation_test.go Outdated Show resolved Hide resolved
@AdityaHegde AdityaHegde force-pushed the feat/watermark-calc-to-resolver branch from a6a60b0 to 313aea8 Compare January 15, 2025 06:32
@AdityaHegde AdityaHegde force-pushed the feat/watermark-calc-to-resolver branch 3 times, most recently from 4ea09db to 96b4732 Compare January 15, 2025 13:12
@AdityaHegde AdityaHegde force-pushed the feat/watermark-calc-to-resolver branch 2 times, most recently from a6d76f9 to 152b243 Compare January 15, 2025 14:18
@AdityaHegde AdityaHegde force-pushed the feat/watermark-calc-to-resolver branch from 152b243 to b21361c Compare January 15, 2025 14:39
runtime/metricsview/executor.go Outdated Show resolved Hide resolved
Comment on lines +51 to +52
var minTime, maxTime, watermark *time.Time
err = rows.Scan(&minTime, &maxTime, &watermark)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ends up sending **time.Time to rows.Scan, might not be well handled by all implementations. Also makes the pointers unsafe to access since they may still be nil. I think this would be safer:

Suggested change
var minTime, maxTime, watermark *time.Time
err = rows.Scan(&minTime, &maxTime, &watermark)
var minTime, maxTime, watermark time.Time
err = rows.Scan(&minTime, &maxTime, &watermark)

Copy link
Collaborator Author

@AdityaHegde AdityaHegde Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for duckdb. It returns null when there are no rows, so we need minTime, maxTime, watermark to be pointers. Perhaps that is why we had different implementation for duckdb. We should probably go back to that. What do you think?

runtime/metricsview/executor_timestamps.go Outdated Show resolved Hide resolved
runtime/queries/metricsview_aggregation.go Outdated Show resolved Hide resolved
runtime/resolvers/metricsview_time_range.go Show resolved Hide resolved

func decodeTimeRangeSummary(row map[string]any) (*runtimev1.TimeRangeSummary, error) {
timeRangeSummary := decodedTimeRangeSummary{} // TODO: move this to a good place
err := mapstructure.Decode(row, &timeRangeSummary)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried using WeakDecode? Then I think it would be able to directly decode ISO strings into time.Time types (but I may be wrong)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wont work by itself. We need to create a decoder with DecoderConfig and StringToTimeHookFunc. I just reused the code from metricsview_aggregation since we will also need for rilltime resolver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants