Skip to content

optimizer: Make compute_fast_path_clusters use fast_path_optimizer #33038

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

Conversation

ggevay
Copy link
Contributor

@ggevay ggevay commented Jul 15, 2025

fast_path_optimizer didn't exist when compute_fast_path_clusters was written, so it uses the full optimizer on each of the other clusters. This can be very slow despite PLAN_INSIGHTS_NOTICE_FAST_PATH_CLUSTERS_OPTIMIZE_DURATION being set to 10ms, because it might happen that optimization for the originally targeted clsuter is fast due to a useful index, but optimizations for any other cluster is slow, due to not having useful indexes. We suspect that this happened yesterday here.

testing: There is an existing test:

"fast_path_clusters": {
"quickstart": {
"index": {
"database": "materialize",
"schema": "public",
"item": "t_primary_idx"
},
"on": {
"database": "materialize",
"schema": "public",
"item": "t"
}
}
},

Motivation

  • This PR fixes a previously unreported bug.

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 A-optimization Area: query optimization and transformation A-ADAPTER Topics related to the ADAPTER layer labels Jul 15, 2025
@ggevay ggevay force-pushed the compute_fast_path_clusters-fast_path_optimizer branch from ea9590d to 079c88a Compare July 15, 2025 14:52
@ggevay ggevay marked this pull request as ready for review July 15, 2025 15:03
@ggevay ggevay requested review from a team as code owners July 15, 2025 15:03
@ggevay ggevay requested a review from aljoscha July 15, 2025 15:03
@ggevay ggevay marked this pull request as draft July 16, 2025 19:34
@ggevay
Copy link
Contributor Author

ggevay commented Jul 16, 2025

Bringing back to draft. There seems to be a very scary CI issue: CI passed on this PR, but if I check out the PR locally and run test/sqllogictest/explain/plan_insights.slt, then it fails. I'll investigate tomorrow.

@ggevay
Copy link
Contributor Author

ggevay commented Jul 18, 2025

Closing, because this approach doesn't work:

  • fast_path_optimizer only works if there was local optimization before, which the PR wanted to skip. This could be fixed by adding NormalizeLets and fusion::Join to the beginning of fast_path_optimizer (in this order).
  • In the peek Optimizer, the create_fast_path_plan call is before fast_path_optimizer, because it just relies on local optimization, but we wanted to skip that. We could hack this by adding an if to local optimization to run fast_path_optimizer instead of the normal local optimization if fast path forcing is set.
  • The probably_fast_path trick on HIR doesn't work as is, because we haven't yet done view inlining when we are on HIR, but we do perform view inlining before fast_path_optimizer is called. I think this could be fixed by moving the view inlining into the else branch of the if fast_path_optimizer in optimize_dataflows, because fast path optimization is enabled exactly when it already looked like fast path even before view inlining (in fact, create_fast_path_plan does a lightly confusing check that no view inlining is needed), so we don't need view inlining for the then branch of that if. We could do this minor change even independently from this PR, because it would just speed up fast path query planning a bit. (But we should probably add a soft assert in the then branch of the if fast_path_optimizer that there is only one object to build.)

https://github.com/MaterializeInc/database-issues/issues/9492 records a better approach for how to fix this (later).

@ggevay ggevay closed this Jul 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ADAPTER Topics related to the ADAPTER layer A-optimization Area: query optimization and transformation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants