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.
| 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.
There was a problem hiding this comment.
Would it make more sense here to use DataType::Null instead of DataType::Utf8 then?
LLDay
force-pushed
the
feat/placeholder-physical-expression
branch
from
787330f to
9b2e08a
Compare
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
9b2e08a to
6e45543
Compare
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Apologies it took a long time to take a look again. I think it generally looks good, though I have some questions on the handling logic for casting.
Also would be good to have this addressed:
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.
| if let Some(placeholder) = expr.as_any().downcast_ref::<PlaceholderExpr>() | ||
| && placeholder.field.is_none() | ||
| { | ||
| expr = expressions::placeholder(&placeholder.id, data_type.clone()); |
There was a problem hiding this comment.
I do wonder what will happen if we then replace the placeholder with a value of type that can't be casted; just a regular execution error? 🤔
| 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.
Would it make more sense here to use DataType::Null instead of DataType::Utf8 then?
Is it ok to introduce some planning config flag to allow physical placeholders planning? If it is false -- we return an error (keep behavior as is). |
Might be quite a niche config 🤔 |
Currently yes as physical placeholders are not supported directly by DF, but this patch target is to add these expressions (and allocate the corresponding protobuf tag to be sure that it will not change), but actually why they are useful for us -- in our fork we replace all placeholders before call |
|
I suppose having a config would be a good middle ground to allow this new functionality whilst keeping things non-surprising for existing users (i.e. placeholders should fail during planning instead of execution) 👍 |
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.