From 3eaec56b630c7126a63b15788ce379f1ca382d3a Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 14 Aug 2025 14:37:31 +0200 Subject: [PATCH 1/2] Remove redundant `plan` from extension's check_invariants In `UserDefinedLogicalNode::check_invariants`, the actual plan to check for invariants is `self`. The `plan` is always `LogicalPlan::Extension` and provides no further information. It's confusing. --- .../core/tests/user_defined/user_defined_plan.rs | 2 +- datafusion/expr/src/logical_plan/extension.rs | 12 ++++-------- datafusion/expr/src/logical_plan/invariants.rs | 2 +- .../substrait/tests/cases/roundtrip_logical_plan.rs | 6 +----- 4 files changed, 7 insertions(+), 15 deletions(-) diff --git a/datafusion/core/tests/user_defined/user_defined_plan.rs b/datafusion/core/tests/user_defined/user_defined_plan.rs index 4d3916c1760ea..f0bf15d3483ba 100644 --- a/datafusion/core/tests/user_defined/user_defined_plan.rs +++ b/datafusion/core/tests/user_defined/user_defined_plan.rs @@ -580,7 +580,7 @@ impl UserDefinedLogicalNodeCore for TopKPlanNode { self.input.schema() } - fn check_invariants(&self, check: InvariantLevel, _plan: &LogicalPlan) -> Result<()> { + fn check_invariants(&self, check: InvariantLevel) -> Result<()> { if let Some(InvariantMock { should_fail_invariant, kind, diff --git a/datafusion/expr/src/logical_plan/extension.rs b/datafusion/expr/src/logical_plan/extension.rs index 5bf64a36a6540..e10b74098b684 100644 --- a/datafusion/expr/src/logical_plan/extension.rs +++ b/datafusion/expr/src/logical_plan/extension.rs @@ -57,7 +57,7 @@ pub trait UserDefinedLogicalNode: fmt::Debug + Send + Sync { fn schema(&self) -> &DFSchemaRef; /// Perform check of invariants for the extension node. - fn check_invariants(&self, check: InvariantLevel, plan: &LogicalPlan) -> Result<()>; + fn check_invariants(&self, check: InvariantLevel) -> Result<()>; /// Returns all expressions in the current logical plan node. This should /// not include expressions of any inputs (aka non-recursively). @@ -241,11 +241,7 @@ pub trait UserDefinedLogicalNodeCore: /// Perform check of invariants for the extension node. /// /// This is the default implementation for extension nodes. - fn check_invariants( - &self, - _check: InvariantLevel, - _plan: &LogicalPlan, - ) -> Result<()> { + fn check_invariants(&self, _check: InvariantLevel) -> Result<()> { Ok(()) } @@ -334,8 +330,8 @@ impl UserDefinedLogicalNode for T { self.schema() } - fn check_invariants(&self, check: InvariantLevel, plan: &LogicalPlan) -> Result<()> { - self.check_invariants(check, plan) + fn check_invariants(&self, check: InvariantLevel) -> Result<()> { + self.check_invariants(check) } fn expressions(&self) -> Vec { diff --git a/datafusion/expr/src/logical_plan/invariants.rs b/datafusion/expr/src/logical_plan/invariants.rs index d8d6739b0e8f0..2d8ed07171026 100644 --- a/datafusion/expr/src/logical_plan/invariants.rs +++ b/datafusion/expr/src/logical_plan/invariants.rs @@ -74,7 +74,7 @@ pub fn assert_executable_invariants(plan: &LogicalPlan) -> Result<()> { fn assert_valid_extension_nodes(plan: &LogicalPlan, check: InvariantLevel) -> Result<()> { plan.apply_with_subqueries(|plan: &LogicalPlan| { if let LogicalPlan::Extension(Extension { node }) = plan { - node.check_invariants(check, plan)?; + node.check_invariants(check)?; } plan.apply_expressions(|expr| { // recursively look for subqueries diff --git a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs index 5ec6663347e2a..6e4e6ea59d074 100644 --- a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs @@ -112,11 +112,7 @@ impl UserDefinedLogicalNode for MockUserDefinedLogicalPlan { &self.empty_schema } - fn check_invariants( - &self, - _check: InvariantLevel, - _plan: &LogicalPlan, - ) -> Result<()> { + fn check_invariants(&self, _check: InvariantLevel) -> Result<()> { Ok(()) } From 2fef5c1e6c071e0bd0431ec2d54287dc2ab1f6a6 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 14 Aug 2025 21:09:34 +0200 Subject: [PATCH 2/2] =?UTF-8?q?empty:=20roll=20the=20dice=20=F0=9F=8E=B2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit