feat: placeholders in execution plans #20169
Conversation
github-actions
bot
added
physical-expr
LLDay
changed the title
|
@askalt, you've already read the code. Could you take another look at it? |
LLDay
force-pushed
the
feat/placeholder-physical-expression
branch
from
0fd0fa2 to
787330f
Compare
|
@Omega359 could you please help to review the patch? There is a basic part of physical placeholders implementation: physical planning support and serde stuff. |
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
It would be good to have justification somewhere in code for this physical expression; e.g. normally this would not be created during planning as logical layer should handle resolution, but what cases this is useful for, etc.
| } | ||
|
|
||
| impl PlaceholderExpr { | ||
| /// Create a new placeholder expression. |
There was a problem hiding this comment.
Personally I find it a bit weird to see new, new_with_X, new_without_Y in succession as constructor methods; would it be better to have as new, new_with_datatype, new_with_field so they are strictly additive semantically or is the new (with datatype) common enough to be the default name?
There was a problem hiding this comment.
Thank you for the review! Yes, I agree with you. I’ve done it as you suggested.
| self.field | ||
| .as_ref() | ||
| .map(Arc::clone) | ||
| .ok_or_else(|| self.execution_error()) |
There was a problem hiding this comment.
nit: this error message would complain about not having value but we're looking at field here?
There was a problem hiding this comment.
I changed the error message text, and along with that I had to update the error message in some tests with placeholders in the logical plans.
| if let Some(placeholder) = expr.as_any().downcast_ref::<PlaceholderExpr>() | ||
| && placeholder.field.is_none() | ||
| { | ||
| // To maintain try_cast behavior, we initially resolve the placeholder with the |
There was a problem hiding this comment.
This seems a bit hacky 🤔
What does it mean to maintain the behaviour?
There was a problem hiding this comment.
I slightly updated the comment. What I meant is that we set the data type to UTF8 so that try_cast can return NULL if the value cannot be converted to the required type. If we try to assign the required type to the placeholder right away, the call will fail with an error instead of returning NULL.
Introduces `PlaceholderExpr`, allowing placeholder parameters to be preserved in the physical plan. Previously, placeholders had to be resolved to literals before physical planning.
LLDay
force-pushed
the
feat/placeholder-physical-expression
branch
from
787330f to
9b2e08a
Compare
4 participants
Rationale for this change
Part of #14342.
Previously, DataFusion required all placeholders to be resolved to literal values before physical planning. This limitation made it difficult to work in systems where physical plans might be cached or reused, or where resolution happens later in the execution pipeline.
By introducing
PlaceholderExprat the physical level, we allow the physical planner to handle unresolved placeholders. This is a logical next step for PR #20009 and part of the effort to improve plan reuse support in DataFusion.What changes are included in this PR?
PlaceholderExpras a new physical expression.PlaceholderExprwhen encounteringExpr::Placeholder.Are these changes tested?
Yes, SLTs have been added, which verifies that placeholders are correctly preserved in physical plans across a wide range of SQL queries.
Are there any user-facing changes?
Yes, users can now generate physical plans for queries containing unresolved placeholders. Attempting to execute these plans without resolving the placeholders will still result in an error, but the planning phase no longer requires their resolution.