andygrove commented on PR #27097: URL: https://github.com/apache/datafusion/pull/27097#issuecomment-6642685640
Thanks @comphead and @avantgardnerio for the reviews. I've pushed the changes and replied to each comment inline. **The `assert!` in `sort_run`.** Done here rather than in a follow-up. Both checks in `sort_run` now return an internal error, with `assert_or_internal_err!` and `assert_eq_or_internal_err!`, so a future mismatch fails the query and leaves the process running. I changed the `assert_eq!` for runs that share no buffers too, since it would abort the process the same way. Without the fix, `sort_runs_sharing_address_with_different_capacities` now fails with the internal error `Assertion failed: reservation.size() + shared.size() >= reserved_bytes` instead of panicking. **The benchmark numbers.** Measuring the counter on `BufferIdMap` showed that the description understated the cost for a one-column batch: it goes from 8.5 ns to 12.8 ns, not 7.3 ns to 8.6 ns, and constructing the larger counter accounts for all of it (7.8 ns to 13.1 ns on its own). I've corrected the description. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
