You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jecsand838 (via GitHub)" <gi...@apache.org> on 2025/09/08 02:38:59 UTC
[PR] Add default values support and refactor `RecordDecoder` to simplify s… [arrow-rs]
jecsand838 opened a new pull request, #8293:
URL: https://github.com/apache/arrow-rs/pull/8293
# Which issue does this PR close?
This work continues arrow-avro schema resolution support and aligns behavior with the Avro spec.
- **Related to**: #4886 (“Add Avro Support”): ongoing work to round out the reader/decoder, including schema resolution and type promotion.
- **Follow-ups/Context**: #8292 (Add array/map/fixed schema resolution and default value support to arrow-avro codec), #8124 (schema resolution & type promotion for the decoder), #8223 (enum mapping for schema resolution). These previous efforts established the foundations that this PR extends to default values and additional resolvable types.
# Rationale for this change
Avro’s specification requires readers to materialize default values when a field exists in the **reader** schema but not in the **writer** schema, and to validate defaults (i.e., union defaults must match the first branch; bytes/fixed defaults must be JSON strings; enums may specify a default symbol for unknown writer symbols). Implementing this behavior makes `arrow-avro` more standards‑compliant and improves interoperability with evolving schemas.
# What changes are included in this PR?
**High‑level summary**
* **Refactor `RecordDecoder`** around a simpler **`Projector`**‑style abstraction that consumes `ResolvedRecord` to: (a) skip writer‑only fields, and (b) materialize reader‑only defaulted fields, reducing branching in the hot path. (See commit subject and record decoder changes.)
**Touched files (2):**
* `arrow-avro/src/reader/record.rs` - refactor decoder to use precomputed mappings and defaults.
* `arrow-avro/src/reader/mod.rs` - add comprehensive tests for defaults and error cases (see below).
# Are these changes tested?
Yes, new integration tests cover both the **happy path** and **validation errors**:
* `test_schema_resolution_defaults_all_supported_types`: verifies that defaults for boolean/int/long/float/double/bytes/string/date/time/timestamp/decimal/fixed/enum/duration/uuid/array/map/nested record and unions are materialized correctly for all rows.
* `test_schema_resolution_default_enum_invalid_symbol_errors`: invalid enum default symbol is rejected.
* `test_schema_resolution_default_fixed_size_mismatch_errors`: mismatched fixed/bytes default lengths are rejected.
These tests assert the Avro‑spec behavior (i.e., union defaults must match the first branch; bytes/fixed defaults use JSON strings).
# Are there any user-facing changes?
N/A
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add projection with default values support to `RecordDecoder` [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on PR #8293:
URL: https://github.com/apache/arrow-rs/pull/8293#issuecomment-3289957838
@alamb Would you be able to review this PR when you have a chance? About half of the code is tests, so ~650 lines of added production code.
Also after this PR, there's two Reader PRs left related to Union support before `arrow-avro` has the functionality to replace `apache_avro` in DataFusion. I'm finishing them both up right now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add projection with default values support to `RecordDecoder` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8293:
URL: https://github.com/apache/arrow-rs/pull/8293#issuecomment-3299687057
Thanks @mbrobbel and @jecsand838
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add projection with default values support to `RecordDecoder` [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #8293:
URL: https://github.com/apache/arrow-rs/pull/8293#discussion_r2349987168
##########
arrow-avro/src/reader/record.rs:
##########
@@ -976,6 +1172,127 @@ fn sign_cast_to<const N: usize>(raw: &[u8]) -> Result<[u8; N], ArrowError> {
Ok(out)
}
+#[derive(Debug)]
+struct Projector {
+ writer_to_reader: Arc<[Option<usize>]>,
+ skip_decoders: Vec<Option<Skipper>>,
+ field_defaults: Vec<Option<AvroLiteral>>,
+ default_injections: Arc<[(usize, AvroLiteral)]>,
+}
+
+#[derive(Debug)]
+struct ProjectorBuilder<'a> {
+ rec: &'a ResolvedRecord,
+ reader_fields: Option<Arc<[AvroField]>>,
+}
+
+impl<'a> ProjectorBuilder<'a> {
+ #[inline]
+ fn try_new(rec: &'a ResolvedRecord) -> Self {
+ Self {
+ rec,
+ reader_fields: None,
+ }
+ }
+
+ #[inline]
+ fn with_reader_fields(mut self, reader_fields: &Arc<[AvroField]>) -> Self {
+ self.reader_fields = Some(reader_fields.clone());
+ self
+ }
+
+ #[inline]
+ fn build(self) -> Result<Projector, ArrowError> {
+ let reader_fields = self.reader_fields.ok_or_else(|| {
+ ArrowError::InvalidArgumentError(
+ "ProjectorBuilder requires reader_fields to be provided".to_string(),
+ )
+ })?;
+ let mut field_defaults: Vec<Option<AvroLiteral>> = Vec::with_capacity(reader_fields.len());
+ for avro_field in reader_fields.iter() {
+ if let Some(ResolutionInfo::DefaultValue(lit)) =
+ avro_field.data_type().resolution.as_ref()
+ {
+ field_defaults.push(Some(lit.clone()));
+ } else {
+ field_defaults.push(None);
+ }
+ }
+ let mut default_injections: Vec<(usize, AvroLiteral)> =
+ Vec::with_capacity(self.rec.default_fields.len());
+ for &idx in self.rec.default_fields.iter() {
+ let lit = field_defaults
+ .get(idx)
+ .and_then(|lit| lit.clone())
+ .unwrap_or(AvroLiteral::Null);
+ default_injections.push((idx, lit));
+ }
+ let mut skip_decoders: Vec<Option<Skipper>> =
+ Vec::with_capacity(self.rec.skip_fields.len());
+ for datatype in self.rec.skip_fields.iter() {
+ let skipper = match datatype {
+ Some(datatype) => Some(Skipper::from_avro(datatype)?),
+ None => None,
+ };
+ skip_decoders.push(skipper);
+ }
+ Ok(Projector {
+ writer_to_reader: self.rec.writer_to_reader.clone(),
+ skip_decoders,
+ field_defaults,
+ default_injections: default_injections.into(),
+ })
+ }
+}
+
+impl Projector {
+ #[inline]
+ fn project_default(&self, decoder: &mut Decoder, index: usize) -> Result<(), ArrowError> {
+ // SAFETY: `index` is obtained by listing the reader's record fields (i.e., from
+ // `decoders.iter_mut().enumerate()`), and `field_defaults` was built in
+ // `ProjectorBuilder::build` to have exactly one element per reader field.
+ // Therefore, `index < self.field_defaults.len()` always holds here, so
+ // `self.field_defaults[index]` cannot panic. We only take an immutable reference
+ // via `.as_ref()`, and `self` is borrowed immutably.
+ if let Some(default_literal) = self.field_defaults[index].as_ref() {
+ decoder.append_default(default_literal)
+ } else {
+ decoder.append_null();
+ Ok(())
+ }
+ }
+
+ #[inline]
+ fn project_record(
+ &mut self,
+ buf: &mut AvroCursor<'_>,
+ encodings: &mut [Decoder],
+ ) -> Result<(), ArrowError> {
+ let n_writer = self.writer_to_reader.len();
+ let n_injections = self.default_injections.len();
+ for index in 0..(n_writer + n_injections) {
+ if index < n_writer {
+ match (
+ self.writer_to_reader[index],
+ self.skip_decoders[index].as_mut(),
+ ) {
Review Comment:
Maybe you can use https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.zip?
##########
arrow-avro/src/reader/record.rs:
##########
@@ -976,6 +1172,127 @@ fn sign_cast_to<const N: usize>(raw: &[u8]) -> Result<[u8; N], ArrowError> {
Ok(out)
}
+#[derive(Debug)]
+struct Projector {
+ writer_to_reader: Arc<[Option<usize>]>,
+ skip_decoders: Vec<Option<Skipper>>,
+ field_defaults: Vec<Option<AvroLiteral>>,
+ default_injections: Arc<[(usize, AvroLiteral)]>,
+}
+
+#[derive(Debug)]
+struct ProjectorBuilder<'a> {
+ rec: &'a ResolvedRecord,
+ reader_fields: Option<Arc<[AvroField]>>,
+}
+
+impl<'a> ProjectorBuilder<'a> {
+ #[inline]
+ fn try_new(rec: &'a ResolvedRecord) -> Self {
+ Self {
+ rec,
+ reader_fields: None,
+ }
+ }
+
+ #[inline]
+ fn with_reader_fields(mut self, reader_fields: &Arc<[AvroField]>) -> Self {
+ self.reader_fields = Some(reader_fields.clone());
+ self
+ }
+
+ #[inline]
+ fn build(self) -> Result<Projector, ArrowError> {
+ let reader_fields = self.reader_fields.ok_or_else(|| {
+ ArrowError::InvalidArgumentError(
+ "ProjectorBuilder requires reader_fields to be provided".to_string(),
+ )
+ })?;
Review Comment:
Maybe the `reader_fields` shouldn't be an `Option` and just an arg of `try_new`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org