Provide user-defined invariants for logical node extensions. #14329
Conversation
github-actions
bot
added
logical-expr
| /// Run invariant checks on the logical plan extension [`TopKPlanNode`]. | ||
| async fn topk_invariants() -> Result<()> { |
There was a problem hiding this comment.
test: demonstrate the basic use case. That user-defined invariants will fail for an invalid extension node.
| #[tokio::test] | ||
| async fn topk_invariants_after_invalid_mutation() -> Result<()> { |
There was a problem hiding this comment.
test: demonstrate a failed invariant check after logical plan mutation (during optimizer run).
| /// Visit the plan nodes, and confirm the [`InvariantLevel::Executable`] | ||
| /// as well as the less stringent [`InvariantLevel::Always`] checks. | ||
| pub fn assert_executable_invariants(plan: &LogicalPlan) -> Result<()> { |
There was a problem hiding this comment.
Documents the existing behavior.
- The
assert_always_invariants()(renamed toassert_always_invariants_at_current_node) assess only the current node, and does not assess the remaining DAG. - whereas the
assert_executable_invariants()(a) visits the subplan, and (b) validates the always and executable.
There was a problem hiding this comment.
The does feel like a blurring of definitions. The previous decision was made based upon minimizing the performance impact; at the time, we wanted the "always" invariants to be be cheap and able to be checked more frequently.
However, as of now the frequency is:
- LP always invariants checked before analyzer
- LP executable (including always) invariants checked:
- after analyzer
- once before all optimizer runs
- once after all optimizer runs
Should I undo this blurring, and have the assert_always_invariants also be recursive?
| // Always invariants | ||
| assert_always_invariants(plan)?; | ||
| assert_valid_extension_nodes(plan, InvariantLevel::Always)?; | ||
|
|
||
| // Executable invariants | ||
| assert_valid_extension_nodes(plan, InvariantLevel::Executable)?; |
There was a problem hiding this comment.
Presuming that the extension nodes are not at the plan root, it does not make sense to check during the assert_always_invariants_at_current_node.
Instead, the extension node invariants are checked during the recursive assertion (a.k.a. assert_executable_invariants). This recursive assertion is done less frequently during the planning -- for example, after all of the optimizers run.
wiedld
force-pushed
the
13525/lp-extension-invariants
branch
from
590c115 to
3836444
Compare
…mutation during an optimizer pass
There was a problem hiding this comment.
Thank you @wiedld -- this looks great. Very nicely tested too.
The only question I have before I think this PR would be ready to merge is if we need both new API functions (invariants and check_invariants).
The other comments are just style comments.
| /// | ||
| /// Refer to [`UserDefinedLogicalNode::check_invariants`](super::UserDefinedLogicalNode) | ||
| /// for more details of user-provided extension node invariants. | ||
| fn assert_valid_extension_nodes(plan: &LogicalPlan, check: InvariantLevel) -> Result<()> { |
There was a problem hiding this comment.
As written I think this this does a separate walk of the tree than assert_subqueries_are_valid
Maybe as a follow on PR we could unify the walks (so the tree gets walked once and all checks applied) rather than two separate plans
… it doesn't make sense for the extension node's checks
|
Thanks again @wiedld |
Which issue does this PR close?
Part of #14029
Rationale for this change
Enable logical plan invariants to be defined by the user.
For this first use case (this PR), the user-defined invariants are provided on the extension nodes.
What changes are included in this PR?
At a high level:
InvariantInvarianton an extension node.Are these changes tested?
Yes
Are there any user-facing changes?
A new trait method is available (
UserDefinedLogicalNode::invariants()), but is not required for existing implementations -- since the default implementation is given.