sdf-jkl commented on PR #10016: URL: https://github.com/apache/arrow-rs/pull/10016#issuecomment-4746165933
> - Ideally the variant builder API should make it structurally impossible to create more than one top-level variant value. This PR doesn't do that. I don't know whether it's even possible to do that, tho, or how hard/invasive it would be It does that. It wasn't obvious because I forgot to add unit tests that shows the behavior - added here https://github.com/apache/arrow-rs/pull/10016/commits/63724871d46326c2a4b8bb1ae4bdc6588d13695a > - I'm not sure this PR blocks finishing an empty variant builder (maybe it does and I just didn't see it in my super-shallow skim) It does that as well. Check the unit tests here https://github.com/apache/arrow-rs/pull/10016/commits/63724871d46326c2a4b8bb1ae4bdc6588d13695a > - There seems to be a potential performance impact (all the careful inlining/cold annotations, flagged benchmark result, etc) That's true, but I inlined/cold'ed as I could and the behavior stays very to original on my machine ( - "it works on my computer" :nerd_face: ) > - It's not immediately obvious that the problem being fixed is causing a lot of pain Some unit tests had errors I fixed here. Example - https://github.com/apache/arrow-rs/pull/10016/changes#diff-3474fdf73b884864c8f3febda29b921f2604e48b61e46e9d78435af4a2972949L469-L473 -- 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]
