Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Minor refactorings around HIR #31124

Merged
merged 3 commits into from
Jan 21, 2025
Merged

Conversation

ggevay
Copy link
Contributor

@ggevay ggevay commented Jan 21, 2025

This PR does a number of trivial refactorings and comment tweaks around HIR.

The first commit renames transform_expr.rs to transform_hir.rs. The old file name came from a time when the HIR types were just named ...Expr, see 0c1e3e8.

The second commit was discussed on slack.

The third commit does some minor comment tweaks in HIR planning.

Motivation

  • This PR is a minor refactoring.

Tips for reviewer

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@ggevay ggevay added the A-ADAPTER Topics related to the ADAPTER layer label Jan 21, 2025
@ggevay ggevay requested review from a team as code owners January 21, 2025 12:20
@ggevay ggevay requested a review from aljoscha January 21, 2025 12:20
Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

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

LGTM, left a comment inline.

@@ -6124,7 +6131,7 @@ impl Visit<'_, Aug> for WindowFuncCollector {
}

fn visit_query(&mut self, _query: &Query<Aug>) {
// Don't go into subqueries. Those will be handled by their own `plan_view_select`.
// Don't go into subqueries. Those will be handled by their own `plan_query`.
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming it's not plan_select_from_where.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, plan_subquery/plan_nested_query calls plan_query. Even though plan_query does eventually call plan_select_from_where, it would be a little misleading to say that a subquery has its own plan_select_from_where, as if that were the whole subquery.

@ggevay ggevay enabled auto-merge January 21, 2025 12:29
@ggevay ggevay merged commit aba254f into MaterializeInc:main Jan 21, 2025
81 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-ADAPTER Topics related to the ADAPTER layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants