klion26 commented on PR #8031: URL: https://github.com/apache/arrow-rs/pull/8031#issuecomment-3143956765
@alamb I think it might be better to stick with the current solution: 1) The solution in the current PR is more complex than the original, but the performance improvement isn't significant; 2) I originally hoped to keep the same implementation for ListBuilder/ObjectBuilder, but it seems that changes to ObjectBuilder might result in performance regressions. @scovich > I mean, yes we can. But given a choice to eliminate a bug surface entirely from the code base, vs. try to validate it with unit tests that could be incomplete and/or go out of sync? I almost always favor eliminating the possibility of bugs over testing for them -- unless the perf and/or complexity difference is large enough to make me question that default choice Thanks for your detailed response. I agree with your point. It's more reliable to fundamentally address the potential for errors. This way, even if there's no test to guarantee this logic for any reason in the future, it will continue to run correctly. I'm a little confused about the benchmark. Why is `PackedU32Iterator` + `extend` the slowest? Is it because it calls multiple `extend`, which results in data movement? (I understand that using `PackedU32Iterator` allows extend to take advantage of `size_hint`). For the unsafe code with `write_u32_at_pos`, I tried it, but it requires further changes; otherwise, it will panic in `write_u32_at_pos` because there is no more room. If we need to benchmark this, I'll try to make this change and benchmark it with current solution. @alamb @scovich Finally, thank you for your detailed review and guidance on these PRs. I've learned a lot from them. Thank you again. -- 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
