Do not attempt to push down calls with offsets #1129

JamesGuthrie · 2022-02-09T15:44:40Z

Description

We don't correctly handle pushdowns with offsets, so we just skip them
instead of returning erroneous results.

Merge requirements

Please take into account the following non-code changes that you may need to make with your PR:

CHANGELOG entry for user-facing changes
~~Updated the relevant documentation~~

niksajakovljevic · 2022-02-09T19:41:44Z

pkg/pgmodel/querier/query_builder.go

+
+	// We can't handle offsets in pushdowns
+	vs, isVectorSelector := queryHints.CurrentNode.(*parser.VectorSelector)
+	if isVectorSelector && (vs.Offset != 0 || vs.OriginalOffset != 0) {


Base on Prometheus docs OriginalOffset is actual offset set from the query so it seems no need to check Offset as that is just derived during query execution.

The plot thickens: this change was not good. We do actually need to validate that both Offset and OriginalOffset are zero.

niksajakovljevic · 2022-02-09T19:44:23Z

pkg/pgmodel/querier/query_builder.go

+	}
+
+	// We can't handle offsets in pushdowns
+	vs, isVectorSelector := queryHints.CurrentNode.(*parser.VectorSelector)


Shouldn't we handle MatrixSelector as well?

I don't think so (but I might be missing something). A MatrixSelector doesn't have an offset, its fields are Range and VectorSelector. The contained VectorSelector may have an offset, and will be handled by this logic.

Hmm MatrixSelector does not embed VectorSelector but Expr which may contain VectorSelector. So I guess in case of MatrixSelector it would not successfully type assert.

In other words in case of MatrixSelector we would need to type assert on it's . VectorSelector field

I've restructured the code by folding the canAttemptPushdown into tryPushDown. This makes the conditions that we're asserting clearer. In particular, it's more visible that we only attempt pushdowns on VectorSelector, which handles your concern here.

niksajakovljevic · 2022-02-09T19:49:46Z

pkg/tests/end_to_end_tests/promql_query_endpoint_test.go

@@ -2506,6 +2506,10 @@ func TestPromQLQueryEndpoint(t *testing.T) {
 			name:  "two pushdowns, same metric different matchers",
 			query: `sum(rate(metric_2{foo = "bar"}[5m]))/sum(rate(metric_2[5m]))`,
 		},
+		{
+			name:  "pushdown with offset",


A bit confused with test naming. Since we don't support pushing down queries with offsets, maybe name it: no pushdown when using offset? Given name gives me an impression that pushdown works with offset

As these tests are primarily used to compare the result that Promscale delivers compared to Prometheus, IMO the description of the test should be independent of what Promscale does or doesn't do. I've updated the test name to delta function over range selector with offset.

antekresic

Couple of nits and a question: do we want to add a TODO to add in the offset support if its something that might be added in the future?

Other than that, I have no problems with the PR itself. Great work 👍

antekresic · 2022-02-10T08:41:15Z

pkg/pgmodel/querier/query_builder.go

+
+	// We can't push down without hints
+	if queryHints == nil || selectHints == nil {
+		return false
+	}
+
+	// We can't do pushdowns without the extension
+	if !extension.ExtensionIsInstalled {
+		return false
+	}
+
+	// We can't handle subqueries in pushdowns
+	if hasSubquery(path) {
+		return false
+	}


The correct Go style for things like this is to use a switch statement, would make the code more compact and easier to read.

TIL, thanks!

antekresic · 2022-02-10T08:42:22Z

pkg/pgmodel/querier/query_builder.go

@@ -242,7 +242,29 @@ func canAttemptPushdown(metadata *promqlMetadata) bool {
 	path := metadata.path // PromQL AST.
 	queryHints := metadata.queryHints
 	selectHints := metadata.selectHints
-	return extension.ExtensionIsInstalled && queryHints != nil && !hasSubquery(path) && selectHints != nil
+
+	// We can't push down without hints


I'm sorry, I have to do this. 😁

Suggested change

// We can't push down without hints

// We can't push down without hints.

No problem! Fixed.

JamesGuthrie · 2022-02-10T08:54:19Z

do we want to add a TODO to add in the offset support if its something that might be added in the future?

I've opened #1130 to track this.

niksajakovljevic

I believe we need to handle MatrixSelector explicitly. Check my related comment.

We don't correctly handle pushdowns with offsets, so we just skip them instead of returning erroneous results.

JamesGuthrie force-pushed the jg/add-failing-offset-pushdown-test branch from ab68577 to e503cb1 Compare February 9, 2022 17:00

JamesGuthrie changed the title ~~Add failing offset pushdown test~~ Do not attempt to push down calls with offsets Feb 9, 2022

JamesGuthrie force-pushed the jg/add-failing-offset-pushdown-test branch from e503cb1 to 28762d6 Compare February 9, 2022 17:03

JamesGuthrie mentioned this pull request Feb 9, 2022

Support offset in pushdowns #1130

Open

JamesGuthrie force-pushed the jg/add-failing-offset-pushdown-test branch 3 times, most recently from b6a5ff6 to 098a214 Compare February 9, 2022 17:24

JamesGuthrie marked this pull request as ready for review February 9, 2022 17:34

JamesGuthrie requested a review from a team as a code owner February 9, 2022 17:34

JamesGuthrie requested review from VineethReddy02, Harkishen-Singh, niksajakovljevic and antekresic and removed request for a team, Harkishen-Singh and VineethReddy02 February 9, 2022 17:34

niksajakovljevic reviewed Feb 9, 2022

View reviewed changes

JamesGuthrie requested a review from niksajakovljevic February 10, 2022 06:29

JamesGuthrie force-pushed the jg/add-failing-offset-pushdown-test branch from f6ad797 to 2c28d79 Compare February 10, 2022 08:07

antekresic approved these changes Feb 10, 2022

View reviewed changes

niksajakovljevic approved these changes Feb 10, 2022

View reviewed changes

JamesGuthrie force-pushed the jg/add-failing-offset-pushdown-test branch 3 times, most recently from 1aadc44 to 30fd31a Compare February 10, 2022 09:38

JamesGuthrie enabled auto-merge (rebase) February 10, 2022 09:39

niksajakovljevic self-requested a review February 10, 2022 09:43

niksajakovljevic suggested changes Feb 10, 2022

View reviewed changes

Do not attempt to push down calls with offsets

54235be

We don't correctly handle pushdowns with offsets, so we just skip them instead of returning erroneous results.

JamesGuthrie force-pushed the jg/add-failing-offset-pushdown-test branch from 0588a42 to 54235be Compare February 10, 2022 10:53

JamesGuthrie requested a review from niksajakovljevic February 10, 2022 10:56

niksajakovljevic approved these changes Feb 10, 2022

View reviewed changes

JamesGuthrie merged commit 9e90a38 into master Feb 10, 2022

JamesGuthrie deleted the jg/add-failing-offset-pushdown-test branch February 10, 2022 13:09

	// We can't push down without hints
	// We can't push down without hints.

Do not attempt to push down calls with offsets #1129

Do not attempt to push down calls with offsets #1129

Uh oh!

Conversation

JamesGuthrie commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Merge requirements

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antekresic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesGuthrie commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niksajakovljevic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JamesGuthrie commented Feb 9, 2022 •

edited

Loading

JamesGuthrie commented Feb 10, 2022 •

edited

Loading