luoyuxia commented on PR #276: URL: https://github.com/apache/paimon-rust/pull/276#issuecomment-4301608822
Thanks for the review! **#1 & #2 (from_avro_bytes intermediate collection):** Fixed. Now deserializes each record individually via `reader.map(|r| from_value(&r?)).collect()`, avoiding the intermediate `Vec<Value>` + `Value::Array` wrapper. Also deduplicated the logic in `manifest.rs` and `index_manifest.rs` to call `from_avro_bytes` directly. **#3 & #4 (get_field_at formatting):** Fixed. Extracted a `let val = ...` local variable so `.and_then(unwrap_value)` is no longer chained directly on the `if let` expression. **#5 (field_index empty slices / O(n·m) fallback):** No change needed. When `records` is empty, `num_rows` is 0 so no iteration happens. When non-empty, `field_index` always returns `Some(idx)`, so the fallback path is never reached in practice. **#6 (build_map_column union unwrapping):** No change needed. As noted, `Value::Map` and `Value::Union` are distinct variants in `apache_avro`, so `unwrap_value` correctly skips unions before reaching the map — this is actually the correct behavior now. **#7 (DataUnexpected empty message):** Fixed. Changed from `""` to `"Failed to process Avro data"`, consistent with the `From<parquet::errors::ParquetError>` pattern. **#8 (HashMap iteration order):** Acknowledged, not a regression — old code also used HashMap. **#9 (index_manifest.rs test manual Union unwrap):** No change needed. Tested and confirmed that `from_value` does **not** automatically unwrap `Value::Union` to a struct — it fails with `"Expected a record or a map. Got: Union(...)"`. The manual `match` is necessary 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
