mmaitre314 commented on code in PR #5102:
URL: https://github.com/apache/arrow-rs/pull/5102#discussion_r1408055814
##########
parquet/src/record/reader.rs:
##########
@@ -274,6 +274,8 @@ impl TreeBuilder {
row_group_reader,
)?;
+ path.push(field_name);
Review Comment:
Thanks for the review -- very much appreciated.
Agreed on pushing-a-value-just-to-pop-it-right-after not being a great
design. I updated the change with an alternative solution which clones the
`path` array instead. It may trigger a few more value copies but the Vec should
typically be short, so likely not a perf concern, and the code looks more
maintainable.
I also looked into the suggestion of returning the Reader but I am not sure
how to make it work without doing a larger refactoring of `reader_tree()` and I
would like to strive to keep the change contained and avoid impacting code
paths other than `REPEATED` to reduce chances of regressions. The challenge I
had with returning Reader is that `Reader::RepeatedReader` created line 277
seems to be assigned to `reader` all the way back line 133, which means that
`path.pop()` line 304 would still be hit.
--
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]
