Skip to content

plan,engine: add partition labels #569

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichaHoffmann
Copy link
Contributor

Add a "PartitionLabelSets" method to remote engine api. This makes sense since a remote engine can have a big labelset but we only need a subset of those to actually perform a partition of all engines. Operating on a smaller labelset there helps us pushing down more expressions, since we know if we lose one of the unimportant labels from the labelset we can still keep pushing down.

@@ -45,6 +45,10 @@ func (l remoteEngine) LabelSets() []labels.Labels {
return l.labelSets
}

func (l remoteEngine) PartitionLabelSets() []labels.Labels {
return l.labelSets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a safe default. Engines who want to specify different partition labels can implement RemoteEngine differently.

@MichaHoffmann MichaHoffmann requested a review from fpetkovski May 21, 2025 10:09
Add a "PartitionLabelSets" method to remote engine api. This makes sense
since a remote engine can have a big labelset but we only need a subset
of those to actually perform a partition of all engines. Operating on a
smaller labelset there helps us pushing down more expressions, since we
know if we lose one of the unimportant labels from the labelset we can
still keep pushing down.

Signed-off-by: Michael Hoffmann <[email protected]>
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/add-partition-labels branch from 960f4d1 to f49bd2f Compare May 21, 2025 19:11
@MichaHoffmann MichaHoffmann requested a review from fpetkovski May 22, 2025 04:29
@@ -57,7 +57,7 @@ func TestDistributedExecution(t *testing.T) {
expr: `sum by (pod) (rate(http_requests_total[5m]))`,
expected: `
sum by (pod) (dedup(
remote(sum by (pod, region) (rate(http_requests_total[5m]))),
remote(sum by (pod, region) (rate(http_requests_total[5m]))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test case where the PartitionLabelSets() and LabelSets() are different?

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.

3 participants