refactor(optimizer): use getters to access plan base, with runtime convention checking

[BugenZhao]

Signed-off-by: Bugen Zhao [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Reviewing these 3 files should be sufficient:

• src/frontend/src/optimizer/plan_node/mod.rs
• src/frontend/src/optimizer/plan_node/plan_base.rs
• src/frontend/src/optimizer/plan_node/stream.rs

---

Towards a type-safer optimizer.

This PR makes all fields in PlanBase private and force the callers to access them by accessing methods with traits like GenericPlanRef, StreamPlanRef, and BatchPlanRef. There're several reasons for doing this:

1. All fields in PlanBase are actually copied from the core when constructing. Exposing the fields then the mutability may lead to inconsistency and confusion.
2. It's nonsense to access a stream-specific field on a logical plan node. Currently we always fill in a dummy value for them, which is unsound. Making fields private gives us more flexibility on the internal structure, enable us to replace the product type with the sum type.
3. By forcing to access the fields through traits, we're able to further constrain the accessing through the type system. Will explore this in the next PRs.

Instructed by the reason 2, this PR introduces a enum Extra for the physical-specific fields. Accessing the fields with a mismatched convention will lead to runtime panic.

risingwave/src/frontend/src/optimizer/plan_node/plan_base.rs

Lines 120 to 121 in 6266b96

	/// Extra fields if the plan node is physical.
	physical_extra: Option<PhysicalExtra>,

risingwave/src/frontend/src/optimizer/plan_node/plan_base.rs

Lines 59 to 64 in 6266b96

	/// Extra fields for physical plan nodes.
	#[derive(Clone, Debug, PartialEq, Eq, Hash)]
	enum PhysicalExtra {
	Stream(StreamExtra),
	Batch(BatchExtra),
	}

Checklist

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

• My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

[codecov]

Codecov Report

Merging #12980 (d5bf548) into main (4851983) will decrease coverage by 0.08%.
Report is 1 commits behind head on main.
The diff coverage is 96.72%.

@@            Coverage Diff             @@
##             main   #12980      +/-   ##
==========================================
- Coverage   68.70%   68.63%   -0.08%     
==========================================
  Files        1495     1495              
  Lines      251092   251102      +10     
==========================================
- Hits       172521   172345     -176     
- Misses      78571    78757     +186     

Flag	Coverage Δ
rust	68.63% <96.72%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/frontend/src/handler/create_mv.rs	93.72% <100.00%> (ø)
...c/frontend/src/optimizer/plan_node/batch_delete.rs	75.00% <100.00%> (-2.15%)	⬇️
...c/frontend/src/optimizer/plan_node/batch_expand.rs	46.34% <100.00%> (ø)
...c/frontend/src/optimizer/plan_node/batch_filter.rs	97.61% <100.00%> (ø)
...ontend/src/optimizer/plan_node/batch_group_topn.rs	58.97% <100.00%> (-2.94%)	⬇️
...frontend/src/optimizer/plan_node/batch_hash_agg.rs	96.84% <100.00%> (ø)
...rontend/src/optimizer/plan_node/batch_hash_join.rs	90.54% <100.00%> (ø)
...ontend/src/optimizer/plan_node/batch_hop_window.rs	75.49% <100.00%> (-0.71%)	⬇️
...c/frontend/src/optimizer/plan_node/batch_insert.rs	54.38% <100.00%> (-0.79%)	⬇️
...rc/frontend/src/optimizer/plan_node/batch_limit.rs	67.34% <100.00%> (ø)
... and 85 more

... and 22 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[st1page]

LGTM

[stdrc]

LSTM

[chenzl25]

Rest LGTM.

[chenzl25]

Putting order here is a little counter-intuitive, since our materialized view could have an order by property i.e. StreamMaterialize theoretically could access its order property, but now it would cause a panic. However, I don't have a better idea right now.

[BugenZhao]

StreamMaterialize theoretically could access its order property, but now it would cause a panic.

Even though we now force a panic when accessing a bad field, all tests are passing. I'm also surprised that this doesn't seem to be a problem.

[chenzl25]

LGTM

[BugenZhao]

Wow. So many conflicts. 🙁