Repository navigation
Automatically check "invariants" #13652
Description
Is your feature request related to a problem or challenge?
I extracted this from #13651 so it was more visible
- Part of [EPIC] A collection of items to improve DataFuson stability (reduce effort required to upgrade) #13648
- Inspired by / copied from Introduce LogicalPlan invariants, begin automatically checking them #13651 from @wiedld
During upgrade, downstream systems often experience issues due to implicit changes (not explicit API changes) of LogicalPlans that DataFusion code begins relying on, and which result in unintended consequences when upgrading to a new version of DataFusion (see #13525).
Describe the solution you'd like
The idea is to make the current implicit assumptions ("Invariants" in more formal language)( explict and automatically check them.
Examples of implicit assumptions:
- Schema column names can't be repeated (this is explicitly mentioned on [DISCUSSION] Making it easier to use DataFusion (lessons from GlareDB) #13525)
- Inputs to
UnionExecmust have the same schema - ...
Describe alternatives you've considered
I like the approach @wiedld took in #13651 :
- define the invariants
- check the invariants for extensible interfaces (which may be user defined)
- throw the error closer to the problem (rather than weird behavior later)
Additional context
Sub tasks:
- Define infrastructure to check LP invariant. PR: Introduce LogicalPlan invariants, begin automatically checking them #13651
- Define infrastructure to check physical plan invariants: WIP: Proposed interface for physical plan invariant checking. #13986
- Define infrastructure for user-defined invariants. See issue: Define extension API for user-defined invariants. #14029
Activity
wiedld commentedon Dec 5, 2024
take
wiedld commentedon Dec 18, 2024
Possible tasks
wiedld commentedon Dec 26, 2024
For the physical optimization invariants, we have that the output physical plan schema cannot change; meaning the output results cannot change, but how we get the results can.
What else should be included as a responsibility/check? Maintain input ordering if required?
(The idea is to have a check, perhaps run in debug mode, that would error if a user-defined physical plan or optimization pass fails to maintain the invariant. Throw error closer to the source when debugging.)
alamb commentedon Dec 26, 2024
Here are some ideas based on bugs we have hit / my memory of what has changed and caused us pain:
wiedld commentedon Dec 27, 2024
My impression was that the plan construction occurred with the LP (as we do), and not by constructing their own de novo physical plan. Is this correct?
If so, then I think the above list of invariants to check would most likely occur at the LP-level (not the physical plan). I can definitely put up a PR for those. Thank you!
Whereas for the physical plan invariants, (not LP), do we want any invariant checking there? Because I looked at the apache docs & physical plan APIs and from (my naive) understanding these are the only two invariants to check after physical plan mutations (a.k.a. PhysicalOptimizerRule applied). Is this correct? 🤔
alamb commentedon Dec 28, 2024
I am sorry -- I don't understand what you are asking (is LP LogicalPlan?) What does a de novo physical plan mean? You mean like creating a
ExecutionPlandirectly (not from aLogicalPlan)?I am not sure what the actual invariants are (part of this project I think is to discover that information)
In my opinion we should be seeking to discover what the existing implicit assumptions are and encode them explicitly in the invariant check. Once we have all the existing assumptions encoded then we can move on to trying to add more assumptions
alamb commentedon Jan 6, 2025
I just discovered that @houqp basically filed this same ticket 2 years ago:
alamb commentedon Jan 6, 2025
I suggest we use this ticket to track the infrastructure for checking invariants (e.g. what @wiedld is doing in #13986) and then claim success.
We can introduce additional invariant checks as we discover them as their own issues / mini projects
wiedld commentedon Jan 6, 2025
Agreed. Modifying this list above, we have infrastructure components of:
alamb commentedon Feb 6, 2025
Task complete!