transform: plumb through `OptimizerFeatures` #25628

aalexandrov · 2024-02-29T00:25:35Z

Make OptimizerConfig available in all parts of the MIR optimizations defined in mz_transform (local and global).

Motivation

This PR adds a known-desirable feature.

Part of MaterializeInc/database-issues#7541.

Tips for reviewer

To follow what is happening it's best to review this one commit at a time.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered. (Relying on the large body of existing tests).
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).
This PR includes the following user-facing behavior changes:
- There are no user-facing behavior changes.

shepherdlybot · 2024-02-29T00:26:39Z

Mitigations

Completing required mitigations increases Resilience Coverage.

Risk Summary:

The pull request poses a high risk, with a score of 81, potentially due to the average age of the files involved, the cognitive complexity within these files, and the changes in executable lines of code. Notably, historical data indicates that pull requests with these characteristics are 107% more likely to introduce bugs compared to the repository's baseline. Additionally, the pull request modifies one file that has recently seen a high number of bug fixes, which could further contribute to its risk level. While the repository's observed bug trend is decreasing, this pull request's risk factors warrant careful review.

Note: The risk score is not based on semantic analysis but on historical predictors of bug occurrence in the repository. The attributes above were deemed the strongest predictors based on that history. Predictors and the score may change as the PR evolves in code, time, and review activity.

Bug Hotspots:
What's This?

File	Percentile
../src/join_implementation.rs	93

ggevay

I wrote a few comments, but still reviewing.

ggevay · 2024-03-08T15:07:21Z

src/transform/src/lib.rs

@@ -398,26 +459,23 @@ pub struct Optimizer {
 impl Optimizer {
    /// Builds a logical optimizer that only performs logical transformations.
    #[deprecated = "Create an Optimize instance and call `optimize` instead."]
-    pub fn logical_optimizer(ctx: &crate::typecheck::SharedContext) -> Self {
+    // TODO: pass TransformCtx instead
+    pub fn logical_optimizer(ctx: &mut TransformCtx) -> Self {


TODO is actually resolved?

Yes, will remove this.

ggevay · 2024-03-08T15:10:26Z

src/transform/src/lib.rs

    /// Transforms can use this field to communicate information outside the result plans.
-    pub dataflow_metainfo: &'a mut DataflowMetainfo,
+    pub df_meta: &'a mut DataflowMetainfo,
 }


Could you please update the doc comment in filter.rs? Or you could maybe even just delete the TransformCtx part of that comment, because it probably has limited usefulness but always annoyingly goes out of sync.

And the same in predicate_pushdown.rs. (typecheck_ctx is missing.)

ggevay · 2024-03-08T15:15:37Z

src/transform/src/lib.rs

+    /// stage.
+    ///
+    /// Used to call [`dataflow::optimize_dataflow`].
+    pub fn global(


I might be missing something, but why not just put this code inside optimize_dataflow (and pass these same arguments to optimize_dataflow instead)?

If you look at the sequence of commits there is an intermediate stage where actually things are as suggested by you, but then I am pulling the TransformCtx::global(...) call outside of optimize_dataflow into the Optimize implementations in `mz_adapter.

Mostly this is done for consistency between the local and the lobal pass, and because I don't want us to touch the signature of optimize_dataflow every time when the TransformCtx contents change—the Optimize implementations in mz_adapter have everything needed to construct a TransformCtx.

ggevay · 2024-03-08T15:17:23Z

src/transform/src/lib.rs

+    /// Used to call [`Optimizer::optimize`] on a
+    /// [`Optimizer::logical_optimizer`] in order to transform a stand-alone
+    /// [`MirRelationExpr`].
+    pub fn local(


Same here: Why not put this into optimize_mir_local?

Same reason as above. Assume that in the next three months we want to touch the definition of TransformCtx three times—I think it's less effort to only modify the five call-sites in src/adapter/src/optimize or think about making the conversion part of some generic trait interface.

I think about it like that:

The Optimzie implementations in src/adapter/src/optimize are a high-level interface that abstracts over the end-to-end entire optimization pipeline for our various statement types. They each own a configuration as a config: OptimizerConfig field which is passed fully formed into the Optimizer::new(...) constructor calls.

The various parts of the pipeline sit behind a low-level API which Optimzie calls. Each primitive works by either calling a function or instantiating an object which contains a primitive-specific optimization. Examples are:

HirRelationExpr::lower(...) which is parameterized by a mz_sql::plan::lowering::Config.

Optimizer::<optimizer_type_ctor>(...) which is parameterized by a TransformCtx.

Optimizer::optimize(...) which is parameterized by a TransformCtx.

Plan::lower_dataflow(...) which is parameterized by an mz_compute_types::plan::lowering::Context`.

The patterns at the moment are not quite as uniform as I would like them to be, but the gist is that each API primitive wants to have it's own context type as a parameter, and the caller should be responsible in forming this paramter from it's own context.

ggevay

LGTM

aalexandrov · 2024-03-08T17:29:37Z

Thanks for the reviews. @teskje / @ggevay I'm certainly open to suggestions how to make this more ergonomic and further reduce the amount of boilerplate code for context forming. I went for regularity at the expense of some code duplication here, hoping that such ideas might emerge naturally in the next 1-2 months.

- Call `TransformCtx::global(...)` in `optimize_dataflow`. - Pass the result as a `optimize_dataflow_relations` parameter.

- Make the `Optimize` implementations in `mz_adapter` responsible for constructing a `TransformCtx` instance. - Pass that instance as a parameter to the `optimize_dataflow` function in `mz_transform`.

Pull the `TransformCtx::local(...)` call one level up from `Optimizer::logical_optimizer` to the caller (`optimize_mir_local`).

- Make the `Optimize` implementations in `mz_adapter` responsible for constructing a `TransformCtx` instance. - Pass that instance as a parameter to the `Optimizer::optimize` method in `mz_transform`.

Share the same `DataflowMetainfo` between the `TransformCtx::local(...)` and the `TransformCtx::global(...)` calls.

aalexandrov requested a review from a team February 29, 2024 00:25

aalexandrov requested a review from a team as a code owner February 29, 2024 00:25

aalexandrov requested review from a team and ParkMyCar February 29, 2024 00:25

aalexandrov marked this pull request as draft February 29, 2024 00:25

aalexandrov self-assigned this Feb 29, 2024

aalexandrov force-pushed the cluster_specific_optimization branch 12 times, most recently from 1685183 to 625420b Compare March 6, 2024 14:12

aalexandrov force-pushed the cluster_specific_optimization branch 6 times, most recently from 5f623b4 to 0c91c00 Compare March 8, 2024 08:04

ggevay reviewed Mar 8, 2024

View reviewed changes

ggevay approved these changes Mar 8, 2024

View reviewed changes

aalexandrov marked this pull request as ready for review March 8, 2024 17:28

aalexandrov changed the title ~~[DNM]: OptimizerFeatures plumbing~~ OptimizerFeatures plumbing into mz_transform Mar 8, 2024

aalexandrov changed the title ~~OptimizerFeatures plumbing into mz_transform~~ transform: plumb through OptimizerFeatures Mar 8, 2024

aalexandrov mentioned this pull request Mar 8, 2024

Equivalence propagation #24155

Merged

5 tasks

aalexandrov added 9 commits March 8, 2024 20:03

transform: pass OptimizerFeatures into TransformCtx

df10f25

transform: pull TransformCtx::global(...) call one level up

b7b6907

- Call `TransformCtx::global(...)` in `optimize_dataflow`. - Pass the result as a `optimize_dataflow_relations` parameter.

transform: add TransformCtx::typecheck_ctx

31e0e6b

optimize: pull TransformCtx::global(...) call one level up

2af9a3f

- Make the `Optimize` implementations in `mz_adapter` responsible for constructing a `TransformCtx` instance. - Pass that instance as a parameter to the `optimize_dataflow` function in `mz_transform`.

transform: rename TransformCtx::{dummy ⇒ local}

41a8cee

transform: pull TransformCtx::local(...) call one level up

5f83f92

Pull the `TransformCtx::local(...)` call one level up from `Optimizer::logical_optimizer` to the caller (`optimize_mir_local`).

optimize: pull TransformCtx::local(...) call one level up

4a0004a

- Make the `Optimize` implementations in `mz_adapter` responsible for constructing a `TransformCtx` instance. - Pass that instance as a parameter to the `Optimizer::optimize` method in `mz_transform`.

optimize: use one DataflowMetainfo instance

952b8a6

Share the same `DataflowMetainfo` between the `TransformCtx::local(...)` and the `TransformCtx::global(...)` calls.

transform: pass TransformCtx to Optimizer constructors

Loading
Loading status checks…

d33bb48

aalexandrov force-pushed the cluster_specific_optimization branch from 0c91c00 to d33bb48 Compare March 8, 2024 18:03

aalexandrov enabled auto-merge March 8, 2024 18:05

aalexandrov added A-optimization A-compute labels Mar 8, 2024

aalexandrov merged commit 310850c into MaterializeInc:main Mar 8, 2024
72 of 73 checks passed

aalexandrov deleted the cluster_specific_optimization branch March 8, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transform: plumb through `OptimizerFeatures` #25628

transform: plumb through `OptimizerFeatures` #25628

aalexandrov commented Feb 29, 2024 •

edited

Loading

shepherdlybot bot commented Feb 29, 2024 •

edited

Loading

ggevay left a comment

ggevay Mar 8, 2024

aalexandrov Mar 8, 2024

ggevay Mar 8, 2024

ggevay Mar 8, 2024

aalexandrov Mar 8, 2024

ggevay Mar 8, 2024

ggevay Mar 8, 2024

aalexandrov Mar 8, 2024

ggevay Mar 8, 2024

aalexandrov Mar 8, 2024

ggevay left a comment

aalexandrov commented Mar 8, 2024 •

edited

Loading

transform: plumb through OptimizerFeatures #25628

transform: plumb through OptimizerFeatures #25628

Conversation

aalexandrov commented Feb 29, 2024 • edited Loading

Motivation

Tips for reviewer

Checklist

shepherdlybot bot commented Feb 29, 2024 • edited Loading

Mitigations

ggevay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggevay left a comment

Choose a reason for hiding this comment

aalexandrov commented Mar 8, 2024 • edited Loading

transform: plumb through `OptimizerFeatures` #25628

transform: plumb through `OptimizerFeatures` #25628

aalexandrov commented Feb 29, 2024 •

edited

Loading

shepherdlybot bot commented Feb 29, 2024 •

edited

Loading

aalexandrov commented Mar 8, 2024 •

edited

Loading