Discuss: Speeding up LogicalPlan manipulation by skipping validation
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
This was written by @sadboy in https://github.com/apache/arrow-datafusion/issues/7775#issuecomment-1856851836 and I think it deserves wider discussion, so I made a new ticket:
As a quick and simple solution, that's what I would recommend, yes.
More fundamentally, I think the contention arises from the de-facto "dual
use" nature of Datafusion's `LogicalPlan`/`Expr` API, which serves two very
different use cases:
1. The DataFrame users, who use DataFusion API to programmtically compose
their queries. In this kind of scenario, you do want to perform eager
input validation and fail early.
2. The "analysis" users, who use `LogicalPlan` as an IR. Datafusion's own
internal use of `LogicalPlan` would mostly fall into this category as
well, including the SQL compiler, the analyzer/optimizers, logical plan
serializers, etc. In this kind of scenario, input sanitation on the plan
node constructors is wasteful as arguments are already known to be
well-formed.
As things currently stand, the constructor methods in `LogicalPlanBuilder`
are largely geared to serve the former use case, but widely shared in the
code paths of the latter. Not an issue when the input queries are small, but
would definitely cause scalability issues when processing large queries
(like we've been experiencing). Having unchecked "dumb" constructors would
be greatly beneficial for perf here, but only if used consistently through
the whole codebase (not exactly a trivial concern, as in general simply
having "unchecked" in the method name would discourage people from using
them).
Ideally, however, I believe these two use cases are different enough that it
would be beneficial to actually separate them statically at the type level.
i.e. have a separate type hierarchy for "DataFramePlan"/"DataFrameExpr",
parallel to `LogicalPlan`/`Expr`. The former would be exclusively used to
capture "end user" input through the DataFrame API, while the latter would
be used exclusively as an internal IR, never directly exposed to the end
user. There would need to be an explicit conversion between the two, but
that would mostly be mechanical and trivial (and where the input sanitation
could take place). The benefit is then you entirely eliminate concerns such
as "should I call the checked or unchecked version of the constructor" when
dealing with logical plan/exprs. In addition, you could "fine tune" each
type hierarchy to better fit its purpose. For example, just off the top of
my head:
* Plan types such as `Expr::Wildcard` can be removed, as they don't serve
any purpose in an IR. The effect is that you can greatly cut down the
"invalid states" in the analyzer/optimizer pipeline, making it much easier
to perform tree transformations
* `Expr::Column` can be changed to index-based rather than name-based (i.e.
`Column { index: usize }`), so you get guaranteed O(1) column resolution
Contributor guide
Research direction
Start by reading the LogicalPlanBuilder constructors and the LogicalPlan/Expr API discussed in the issue. Compare the DataFrame and analysis use cases described there, then determine whether unchecked constructors or separate DataFramePlan/DataFrameExpr types are appropriate. The issue does not define a concrete implementation scope or a clear done condition.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100