Add Projection::try_new and Projection::try_new_with_schema
#2900
Conversation
github-actions
bot
added
logical-expr
andygrove
changed the title
Projection::try_newProjection::try_new and Projection::try_new_with_schema
andygrove
changed the title
Projection::try_new and Projection::try_new_with_schemaProjection::try_new and Projection::try_new_with_schema and fix invalid projection in common_subexpr_eliminate
| let mut schema = DFSchema::new_with_metadata(fields, HashMap::new())?; | ||
| schema.merge(input.schema()); |
There was a problem hiding this comment.
This is the fix for #2907. This code is incorrect and is producing a projection output schema containing all the fields from the input schema. The code now just relies on the logic in Projection::try_new to produce a valid schema based on the projection expressions.
No, it wasn't the fix.
This reverts commit 28f07a0.
andygrove
changed the title
Projection::try_new and Projection::try_new_with_schema and fix invalid projection in common_subexpr_eliminateProjection::try_new and Projection::try_new_with_schema
|
@jdye64 This is the first step in tackling the index out-of-bounds issue. Could you review? |
|
Sure thing, I'm cherry picking a few things to try on my local setup. If that all goes as planned will review the actual changes. |
jdye64
left a comment
jdye64
left a comment
There was a problem hiding this comment.
LGTM, will be handy to have this check and more clear errors.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks like a nice improvement to me (and we now have a test!)
To be clear this doesn't fix any underlying bugs (yet), it just is refactoring the code to make it easer to test, right (which I think is great ❤️ )
👍
| } | ||
|
|
||
| #[test] | ||
| fn projection_expr_schema_mismatch() -> Result<()> { |
|
Benchmark runs are scheduled for baseline = 9401d6d and contender = 034678b. 034678b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Which issue does this PR close?
Part of #2907
Rationale for this change
I was hitting the following error when running a query due to
common_subexpr_eliminatecreating an invalid projection and this was hard to track down.Rather than create the Projection struct directly, it is better to call a
try_newconstructor that can:This gave a better error:
What changes are included in this PR?
Introduce
try_newconstructor and use that instead of creating structs directlyAre there any user-facing changes?
No, this is not a breaking change.