jecsand838 commented on code in PR #7954:
URL: https://github.com/apache/arrow-rs/pull/7954#discussion_r2216766988
##########
arrow-avro/src/reader/record.rs:
##########
@@ -431,12 +422,18 @@ impl Decoder {
let nanos = (millis as i64) * 1_000_000;
builder.append_value(IntervalMonthDayNano::new(months as i32,
days as i32, nanos));
}
- Self::Nullable(nullability, nulls, e) => {
- let is_valid = buf.get_bool()? == matches!(nullability,
Nullability::NullFirst);
- nulls.append(is_valid);
- match is_valid {
- true => e.decode(buf)?,
- false => e.append_null(),
+ Self::Nullable(order, nb, child) => {
+ let branch = buf.get_int()?;
+ let is_not_null = match order {
+ Nullability::NullFirst => branch != 0,
+ Nullability::NullSecond => branch == 0,
+ };
+
+ nb.append(is_not_null);
+ if is_not_null {
+ child.decode(buf)?;
+ } else {
+ child.append_null();
Review Comment:
Small suggestion here.
Since we only need to distinguish between zero and non-zero cases. I think
we can get away with only reading the raw varint and checking if it's zero
without the need to perform the full zigzag decoding. i.e. `buf.read_vlq()`
instead of `buf.get_int()`.
Also I just renamed `child` to `encoding` to align with the other branches.
```suggestion
Self::Nullable(order, nb, encoding) => {
let branch = buf.read_vlq()?;
let is_not_null = match *order {
Nullability::NullFirst => branch != 0,
Nullability::NullSecond => branch == 0,
};
nb.append(is_not_null);
if is_not_null {
encoding.decode(buf)?;
} else {
encoding.append_null();
```
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
