Fix window expr deserialization #10506
Conversation
There was a problem hiding this comment.
Thank you for this PR @lewiszlw -- I can't quite understand what it is fixing. Is there any way you can add a unit test to the round trip serialization tests that fails prior to the code change but passes afterwards?
I think that would make it easier to understand what bug was fixed as well as ensure we don't accidentally break it again
Somewere in https://github.com/apache/datafusion/blob/main/datafusion/proto/tests/proto_integration.rs perhaps?
I updated desc and physical window plan proto test. |
| @@ -253,8 +253,7 @@ fn roundtrip_nested_loop_join() -> Result<()> { | |||
| fn roundtrip_window() -> Result<()> { | |||
| let field_a = Field::new("a", DataType::Int64, false); | |||
| let field_b = Field::new("b", DataType::Int64, false); | |||
| let field_c = Field::new("FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW", DataType::Int64, false); | |||
| let schema = Arc::new(Schema::new(vec![field_a, field_b, field_c])); | |||
| let schema = Arc::new(Schema::new(vec![field_a, field_b])); | |||
There was a problem hiding this comment.
this schema will be used as input schema, see line 310. I don't think the input schema should contains window field.
Which issue does this PR close?
Fix window expr deserialization bug which introduced in #9686.
We encountered the error like this when deserializing window plan from proto.
The error thrown at https://github.com/apache/datafusion/blob/main/datafusion/physical-plan/src/windows/mod.rs#L188.
If I understand correctly, now
create_window_exprfunction needs output schema of window plan instead of the schema of window plan input. So we should build extended schema which contains input schema and window fields.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?