MQE time related functions #10486

lamida · 2025-01-20T23:36:57Z

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

pkg/streamingpromql/functions.go

pkg/streamingpromql/operators/functions/times.go

pkg/streamingpromql/testdata/upstream/functions.test

pkg/streamingpromql/operators/functions/times.go

pkg/streamingpromql/testdata/ours/functions.test

charleskorn · 2025-01-28T04:34:33Z

pkg/streamingpromql/functions.go

@@ -61,6 +61,37 @@ func SingleInputVectorFunctionOperatorFactory(name string, f functions.FunctionO
 	}
 }

+func TimeFunctionOperatorFactory(name string, f functions.FunctionOverInstantVectorDefinition) InstantVectorFunctionOperatorFactory {


Given this is only used by TimeTransformationFunctionOperatorFactory, I would move this inside that function - I don't think the separation is useful, and they're always used together.

Should be inlined in 16843e7

charleskorn

LGTM modulo nits

pkg/streamingpromql/operators/time.go

charleskorn · 2025-01-29T00:06:05Z

pkg/streamingpromql/functions.go

+		if len(args) > 1 {
+			// Should be caught by the PromQL parser, but we check here for safety.
+			return nil, fmt.Errorf("expected 0 or 1 argument for %s, got %v", name, len(args))
+		}
+
+		var inner types.InstantVectorOperator
+		if len(args) > 0 {
+			var ok bool
+			// time based function expect instant vector argument
+			inner, ok = args[0].(types.InstantVectorOperator)
+			if !ok {
+				// Should be caught by the PromQL parser, but we check here for safety.
+				return nil, fmt.Errorf("expected an instant vector argument for %s, got %T", name, args[0])
+			}
+
+		} else {
+			// if the argument is not provided, it will default to vector(time())
+			inner = scalars.NewScalarToInstantVector(operators.NewTime(timeRange, memoryConsumptionTracker, expressionPosition), expressionPosition)
+		}


[nit] This could be simplified to a structure like:

if len(args) == 0 { ... } else if len(args) == 1 { ... } else { return nil, fmt.Errorf("expected 0 or 1 argument for %s, got %v", name, len(args)) }

Updated in 1b91d56

charleskorn

Also just noticed there are a bunch of test cases in pkg/streamingpromql/testdata/upstream/at_modifier.test that can now be enabled (eg. those starting around line 200) - could you please enable these?

(tools/check-for-disabled-but-supported-mqe-test-cases should find them all for you)

charleskorn · 2025-02-03T05:25:02Z

pkg/streamingpromql/testdata/upstream/name_label_dropping.test

These test cases aren't actually supported by MQE - we haven't yet implemented the experimental delayed name dropping feature, and don't intend to any time soon.

Reverted in 2a4762a . I added this because tools/check-for-disabled-but-supported-mqe-test-cases said it is supported, although it was saying it for non-times function.

Unfortunately tools/check-for-disabled-but-supported-mqe-test-cases doesn't recognise that they're not really supported (perhaps we should modify it to understand this)

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida force-pushed the lamida/mqe-time-functions branch from 2cf7ef4 to 5ebc1ff Compare January 22, 2025 13:19

lamida marked this pull request as ready for review January 22, 2025 18:03

lamida requested a review from a team as a code owner January 22, 2025 18:03

charleskorn reviewed Jan 23, 2025

View reviewed changes

lamida requested a review from charleskorn January 27, 2025 17:31

charleskorn reviewed Jan 28, 2025

View reviewed changes

lamida requested a review from charleskorn January 28, 2025 10:41

charleskorn approved these changes Jan 29, 2025

View reviewed changes

charleskorn reviewed Jan 29, 2025

View reviewed changes

charleskorn mentioned this pull request Jan 29, 2025

Mimir Query Engine #10067

Open

charleskorn reviewed Feb 3, 2025

View reviewed changes

lamida and others added 19 commits February 3, 2025 15:07

Implement time functions

664d6e6

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add more times function

6f64881

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Try restructuring the functions

a5eab6d

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Implement time related functions

7aa5a04

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Remove unused field

e534c60

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Remove commented import

dcc2dd6

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add comment for readability

7d7f1dd

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Missed to uncomment test

3a2ce98

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add missing license header

243ba93

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Refactor the time function name

7f1f3a5

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Missed function to uncomment

83b94b8

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Split the file between times function and time operator

4d99610

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Push new times function

9c3f90b

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Update pkg/streamingpromql/functions.go

40a46e6

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Move ok declaration inside if

1555c85

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Remove unneeded block

8c7045b

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Return histogram slice to pool in defer

3b6dbf8

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Fix unused parameter

532455c

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add range query test for times function

93f6428

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida and others added 13 commits February 3, 2025 15:07

Add more time tests

c7c317e

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Update pkg/streamingpromql/testdata/ours/functions.test

154d75e

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Update pkg/streamingpromql/testdata/ours/functions.test

c9fc0d1

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Update pkg/streamingpromql/testdata/ours/functions.test

0773d2e

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Inline TimeFunctionOperatorFactory

f180288

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Fix the test

962fbc2

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Return histogram to the poll

58f656b

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Update pkg/streamingpromql/operators/time.go

5c904e1

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Update pkg/streamingpromql/operators/time.go

1ca10a7

Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

Refactor if structure for TimeTransformationFunctionOperatorFactory

a066900

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Cast int step to int64

d3f76b4

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Enable more supported tests

7b746eb

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Revert name_label_dropping.test

512365d

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida force-pushed the lamida/mqe-time-functions branch from 2a4762a to 512365d Compare February 3, 2025 07:07

Add times functions to TestFunctionDeduplicateAndMerge

c8c5441

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida merged commit 84f72f7 into main Feb 3, 2025
28 checks passed

lamida deleted the lamida/mqe-time-functions branch February 3, 2025 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MQE time related functions #10486

MQE time related functions #10486

Uh oh!

lamida commented Jan 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

charleskorn Jan 28, 2025

Uh oh!

lamida Jan 28, 2025

Uh oh!

charleskorn left a comment

Uh oh!

Uh oh!

Uh oh!

charleskorn Jan 29, 2025

Uh oh!

lamida Feb 3, 2025

Uh oh!

charleskorn left a comment

Uh oh!

charleskorn Feb 3, 2025

Uh oh!

lamida Feb 3, 2025

Uh oh!

charleskorn Feb 3, 2025

Uh oh!

Uh oh!

Uh oh!

MQE time related functions #10486

MQE time related functions #10486

Uh oh!

Conversation

lamida commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

charleskorn Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

lamida Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

charleskorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

charleskorn Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

lamida Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

charleskorn left a comment

Choose a reason for hiding this comment

Uh oh!

charleskorn Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

lamida Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

charleskorn Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lamida commented Jan 20, 2025 •

edited

Loading