adriangb commented on code in PR #24165:
URL: https://github.com/apache/datafusion/pull/24165#discussion_r3737779584
##########
datafusion/proto-models/src/generated/pbjson.rs:
##########
@@ -9063,6 +9066,11 @@ impl serde::Serialize for HashJoinExecNode {
if let Some(v) = self.dynamic_filter.as_ref() {
struct_ser.serialize_field("dynamicFilter", v)?;
}
+ if let Some(v) = self.fetch.as_ref() {
Review Comment:
Correct, and confirmed in the generated code: pbjson's deserializer ends its
field match with `_ => Err(serde::de::Error::unknown_field(value, FIELDS))`, so
an older reader will reject JSON containing fetch.
That said, the serializer only emits the field when it's set
```rust
if let Some(v) = self.fetch.as_ref() {
struct_ser.serialize_field("fetch", v)?;
}
```
So a new writer's JSON is unchanged for any plan without a fetch, and old
readers only break on plans that actually carry a limit pushed into the join
(which would have otherwise been a serious runtime bug). Binary stays both
backward and forward compatible in all cases.
Making JSON forward-compatible would mean changing the unknown-field policy
globally (deny → ignore), which feels like its own discussion rather than
something to settle in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
