You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "elad-yosifon (via GitHub)" <gi...@apache.org> on 2025/07/16 23:04:00 UTC
[PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
elad-yosifon opened a new pull request, #227:
URL: https://github.com/apache/avro-rs/pull/227
fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)]
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3106241422
@elad-yosifon While working on the serde-driven deserialization I've realized that `skip_field()` needs to serialize something, otherwise the deserialization uses the wrong Avro Schema of the next field.
So I've added logic to use the RecordField's `default` value (an Option).
Now only the `multiple skipped fields` test fails due to the `skip` attribute. For some reason Serde does not call `skip_field()` for this fiels ...
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3082819739
I applied the suggestions by Copilot (compare the serialized record with the deserialized one) and the assertion fails ... The `Some(1)` is lost ... I'll try to debug it soon!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #227:
URL: https://github.com/apache/avro-rs/pull/227
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "elad-yosifon (via GitHub)" <gi...@apache.org>.
elad-yosifon commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3106274153
@martin-g maybe it is a niche optimization, as all of the NONE optionals are at the tail, so one might think there is no need to indicate the skip.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "jdarais (via GitHub)" <gi...@apache.org>.
jdarais commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3121254182
Ah, those are some good points...
> Supporting skip is OK.
> But what to do with skip_serializing and skip_deserialing ?! We will need to have a field for such fields in the Avro Schema but Serde does not call skip_field() when skip_serializing is used ...
>
I think `skip_serializing` and `skip_deserializing` are also doable. Since the Serializer is the one with the schema, the schema is decoupled from the struct implementing `Serialize`, so you just have to make sure that the schema you use to serialize doesn't include the field with the `skip_serializing` attribute. (Of course, reading the value back would require a different schema that includes the field :slightly_frowning_face: but it is doable with different read and write schemas. If you use `skip_serializing` but not `skip_deserializing`, then maybe that's just what you're signing yourself up for.)
`skip_deserializing` should work more easily, since if we first read the value into an avro `Value` using the schema, then when we convert from `Value` to the struct using the `de.rs` implementation, it should be fine to ignore the value since doing so won't cause issues with parsing the rest of the record in the same way that ignoring a field does when serializing.
> skip_field does not provide the value, so we use the default from the Avro Schema. This might need some more work! At the moment it is treated as an Option but probably it should be unpacked.
Ah, that's a very good point. That's a bummer that the value isn't provided in `skip_field`. Yeah, if the default value is `None` (meaning no default value was specified) then we're kind of stuck, since there's no way to know what should go in that field. Might be good to just return an `Err` in that case.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3098094547
> @martin-g any ETA on this?
Dunno.
I started working on it at https://github.com/apache/avro-rs/pull/237
Feel free to send PRs against my branch!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "jdarais (via GitHub)" <gi...@apache.org>.
jdarais commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3113920339
Sorry about the delayed response, I've been traveling. On this question:
> For full roundtrip we will need a Serde-driven deserialization too.
ser.rs and de.rs are Schema-driven impls.
ser_schema.rs is Serde-driven serialization. We will need de_schema.rs (or some better names) for Serde-driven impl.
I was looking into creating a serde-driven deserializer, but in the end it seemed easier to first deserialize into an avro `Value`, which provides some flexibility that helps with schema resolution (if the reader schema is different from the writer schema,) and then use the existing `de.rs` implementation to convert from `Value` to the final type.
I feel like this approach should still work for a round-trip of serialization and deserialization. If it doesn't then I think that would mean that either the serde-driven serialization or the schema-driven deserialization has a bug, or simply that the default value used for serialization, (provided by the schema,) is different from the default value used for deserialization, (which as far as I can tell uses `Default::default` and bypasses deserialization logic altogether.)
In general, the `skip_serializing_if` attribute seems to be at odds with serialization to Avro, since Avro structs give no accommodation for simply skipping serialization of a field: all fields of a struct must be present. (Using the default value for the field provided by the schema is a reasonable workaround, since that simulates omiting the field, though per the [spec](https://avro.apache.org/docs/1.12.0/specification/#schema-record), the default value is "only used when reading instances that lack the field for schema evolution purposes. The presence of a default value does not make the field optional at encoding time." Also, with the changes from this PR, if the default value is not present, then the struct will be serialized incorrectly.)
Using the `skip` attribute would be easier to use with Avro since you can just ensure that the skipped field isn't included in the Avro schema used to write.
Now that I think about it, I'm guessing that `skip_serializing_if` is mostly useful for removing attributes that are just `null` or the default value so they don't take up unnecessary space in the serialized payload. It might work to just have the `skip_field` implementation do the exact same thing as `serialize_field`, effectively always including the field, since you're not allowed to omit the field anyway.
On a different topic: I see from the PR that there was some logic in there before that ensured that struct fields were written in the correct order, (they must be written in the order in which they appear in the schema,) even if the order of the fields in the rust struct is different from what's in the schema. It'd be nice to get that back in.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "elad-yosifon (via GitHub)" <gi...@apache.org>.
elad-yosifon commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3108452510
That’s good for now 👍
On Wed, 23 Jul 2025 at 16:08 Martin Grigorov ***@***.***>
wrote:
> *martin-g* left a comment (apache/avro-rs#227)
> <https://github.com/apache/avro-rs/pull/227#issuecomment-3108195507>
>
> @elad-yosifon <https://github.com/elad-yosifon> I'd be OK to merge this
> PR in its current state if it is enough for your use case.
> It supports skip_serializing_if but it does not support skip_serializing/
> skip!
> The latter will probably be supported once #237
> <https://github.com/apache/avro-rs/pull/237> is finished (but it is a ton
> of work...).
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/avro-rs/pull/227#issuecomment-3108195507>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAWZ45CIUXPDYOTJ5SHKR6L3J6CLPAVCNFSM6AAAAACBWGU2ECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCMBYGE4TKNJQG4>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3116660601
Hi @jdarais !
> I was looking into creating a serde-driven deserializer, but in the end it seemed easier to first deserialize into an avro `Value`, which provides some flexibility that helps with schema resolution (if the reader schema is different from the writer schema,) and then use the existing `de.rs` implementation to convert from `Value` to the final type.
The problem here is that the users expect that the schema-driven deserialization in de.rs should take into account the Serde attributes, like the `skip` ones. But maybe as you said we just have to ignore them. Otherwise we will face issues later with attributes like `flatten`, `remote`, etc.
But in that case I have no good answer to "I want to use the same serde mechanism for JSON, CSV, Avro, ...) (https://github.com/apache/avro-rs/pull/227#issuecomment-3097968926).
> Using the `skip` attribute would be easier to use with Avro since you can just ensure that the skipped field isn't included in the Avro schema used to write.
Supporting `skip` is OK.
But what to do with `skip_serializing` and `skip_deserialing` ?! We will need to have a field for such fields in the Avro Schema but Serde does not call `skip_field()` when `skip_serializing` is used ...
> It might work to just have the `skip_field` implementation do the exact same thing as `serialize_field`, effectively always including the field, since you're not allowed to omit the field anyway.
We kinda do this at the moment.
* `fn serialize_field<T>(&mut self, key: &'static str, value: &T)`
* `fn skip_field(&mut self, key: &'static str)`
`skip_field` does not provide the `value`, so we use the `default` from the Avro Schema. This might need some more work! At the moment it is treated as an Option but probably it should be unpacked.
> On a different topic: I see from the PR that there was some logic in there before that ensured that struct fields were written in the correct order, (they must be written in the order in which they appear in the schema,) even if the order of the fields in the rust struct is different from what's in the schema. It'd be nice to get that back in. (I realize there should have been a test for it.)
You are totally right here!
It should work fine when the schema is derived but it will break bad at deserialization time if the schema is created manually!
For now we can add a note to the documentation!
I removed all logic that didn't break any test :-/
It could be returned with the respective unit tests!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3106346373
The `y` field is tagged with `skip_serializing_if` and `skip`. The next field (`z`) has just `skip_serializing_if`
https://github.com/apache/avro-rs/pull/227/files#diff-d3a12aeda34e86ab5d302f29a1673cef4fd727f5ad5d9bab18380817a88a47d5R90-R94
The `skip_field()` method is called directly for `z`!
Removing `skip_serializing_if` for `y` does not change anything.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3107266247
`skip_serializing_if` leads to a call to `skip_field`!
But `skip_serializing` (**without** the `_if`!) does not!
`skip` attribute just sets `skip_serializing` and `skip_deserializing`
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #227:
URL: https://github.com/apache/avro-rs/pull/227#discussion_r2212102678
##########
avro/src/ser_schema.rs:
##########
@@ -384,6 +384,11 @@ impl<W: Write> ser::SerializeStruct for SchemaAwareWriteSerializeStruct<'_, '_,
}
}
Review Comment:
[nitpick] Add a doc comment explaining why `skip_field` increments `item_count`, so future maintainers understand how skipped fields affect serialization order.
```suggestion
/// Skips a field during serialization.
///
/// Incrementing `item_count` ensures that the serialization order
/// remains consistent with the schema, even when fields are skipped.
```
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3083845776
This won't work with the current version of avro-rs.
With your PR the field is properly skipped, i.e. nothing is serialized for this field.
But Avro's Reader uses the T::schema() to lead the deserialization of the data and wrongly reads the `y`'s value into `x` field and `z`'s value into `y`.
For full roundtrip we will need a Serde-driven deserialization too.
ser.rs and de.rs are Schema-driven impls.
ser_schema.rs is Serde-driven serialization. We will need de_schema.rs (or some better names) for Serde-driven impl.
@jdarais Do you agree with me here ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3116696957
> It should work fine when the schema is derived but it will break bad at deserialization time if the schema is created manually! For now we can add a note to the documentation!
https://github.com/apache/avro-rs/pull/242
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3106266543
I have some other things to do now but I will continue debugging it later.
Any help/hints why Serde does not call the method are welcome!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "elad-yosifon (via GitHub)" <gi...@apache.org>.
elad-yosifon commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3106259790
@martin-g thanks for the update.
This seems odd, but at least the test caught it :)
Are you done with the debugging, or still at it?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3108195507
@elad-yosifon I'd be OK to merge this PR in its current state if it is enough for your use case.
It supports `skip_serializing_if` but it does not support `skip_serializing`/`skip`!
The latter will probably be supported once https://github.com/apache/avro-rs/pull/237 is finished (but it is a ton of work...).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix #226: Index-Out-Of-Bounds panic when using #[serde(skip_serializing_if=..)] [avro-rs]
Posted by "elad-yosifon (via GitHub)" <gi...@apache.org>.
elad-yosifon commented on PR #227:
URL: https://github.com/apache/avro-rs/pull/227#issuecomment-3097968926
@martin-g any ETA on this? I do understand that my PR is not fixing anything.
My current situation is that I have a struct that derives serde::Serialize, and I need to use the same struct for CSV, JSON, and AVRO. Since I am trying to avoid redundant fields (mostly for JSON), I cannot remove the `skip_serializing_if` directive AFAIK.
Is there a workaround you can suggest?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org