Skip to content

Conversation

@ashwanthgoli
Copy link
Contributor

@ashwanthgoli ashwanthgoli commented Jun 4, 2025

What this PR does / why we need it:

Adds physical and logical plan nodes for range aggregation. We could have each aggregation operation represented as a different node but this feels like a good starting point.

Physical plan is also called RangeAggregation, it does not say how the aggregation is being performed. This will be updated in a follow-up once we have a basic version of the execution operator.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@ashwanthgoli ashwanthgoli marked this pull request as ready for review June 4, 2025 14:46
@ashwanthgoli ashwanthgoli requested a review from a team as a code owner June 4, 2025 14:46
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Nits, then LGTM.

id string

Table Value // The table relation to aggregate.
PartitionBy []ColumnRef // The columns to partition by.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I wouldn't refer to this as partitioning, as that insinuates we're grouping by values:

  • Partitioning creates logical or physical boundaries in your data based on column values
  • Ordering arranges rows in a specific sequence
  • Projection filters which columns appear in results

Instead I'd call this soemthing like Columns, Projection, or ProjectedColumns.

Copy link
Contributor Author

@ashwanthgoli ashwanthgoli Jun 5, 2025

Choose a reason for hiding this comment

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

Partitioning creates logical or physical boundaries in your data based on column values

^ this is what i was intending it to be

  • Range aggregation would partition the data using the column values
  • For each partition, it runs the aggregation over the time window
    • Aggregation is run for each step if its a range query

I am of the idea that we would implicitly only project the partition columns (or all stream columns if partitioning is not defined), but we can make this explicit

@ashwanthgoli ashwanthgoli merged commit e1206fb into main Jun 6, 2025
65 checks passed
@ashwanthgoli ashwanthgoli deleted the planning-count-over-time branch June 6, 2025 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants