Skip to content

Conversation

rubywtl
Copy link
Contributor

@rubywtl rubywtl commented Sep 5, 2025

What this PR does:
This PR implements a logical plan fragmenter for distributed query execution. The fragmenter identifies fragments based on remote node markers in the logical plan and creates independent execution units that can be distributed across queriers. It maintains execution dependencies between parent and child fragments

Which issue(s) this PR fixes:
related to distributed query execution proposal

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@rubywtl rubywtl force-pushed the scheduler/logicalplan-fragmenter branch from 8b4f789 to 02fd607 Compare September 5, 2025 22:02
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks for the work. There is some room for improvement in the test but it looks good in general.


if len(fragments) > 0 {
return fragments, nil
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. You can remove this else as we already return above


require.NoError(t, err)
require.Equal(t, tc.expectedFragments, len(res))

// check the metadata of the fragments of binary expressions
if len(res) == 3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this check? This only works for the second test case?

query: "sum(rate(http_requests_total{job=\"api\"}[5m])) + sum(rate(http_requests_total{job=\"web\"}[5m])) + sum(rate(http_requests_total{job=\"cache\"}[5m])) + sum(rate(http_requests_total{job=\"db\"}[5m]))",
start: now,
end: now,
expectedFragments: 7,
Copy link
Contributor

Choose a reason for hiding this comment

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

The test should try to compare the output fragments instead of number of fragments to be safer

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

Successfully merging this pull request may close these issues.

2 participants