Skip to content

New flavor of injected expression#416

Merged
zachmu merged 7 commits intomainfrom
zachmu/agg2
May 30, 2025
Merged

New flavor of injected expression#416
zachmu merged 7 commits intomainfrom
zachmu/agg2

Conversation

@zachmu
Copy link
Member

@zachmu zachmu commented May 30, 2025

No description provided.

Copy link

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

InjectedExpr gets a bit more complicated to use though. Seems like you could probably get away with adding OrderBy directly to InjectedExpr, too, and avoid the new type, but I understand having the type makes it convenient to switch on type. The biggest design point I can think of there is if we think we'll end up adding many more of these extensions, and if the extensions will have cases where we need an OrderBy and some other field that comes from a different extension type. That ends up getting messy with many similar types obviously.

Overall seems reasonable though, and it sounds like we want to revisit aggregation in the future as we better understand what Doltgres needs, so I'm on board.

@zachmu zachmu merged commit bfd5228 into main May 30, 2025
4 checks passed
@Hydrocharged Hydrocharged deleted the zachmu/agg2 branch January 9, 2026 11:10
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.

2 participants