andygrove commented on PR #23766: URL: https://github.com/apache/datafusion/pull/23766#issuecomment-5049312854
Thanks for the review @alamb. > I wonder if we have something similar with base64 🤔 I checked, and happily no: there is exactly one hand-written base64 site, `BASE64_ENGINE` / `BASE64_ENGINE_PADDED` in `encoding/inner.rs`. Everything else that greps for base64 is generated proto code going through `pbjson`'s own helper. Hex was the outlier because `to_hex`, Spark's `hex`, `md5`, `sha1` and `sha2` had each independently grown a lookup table, and they had drifted into two different levels of optimization. Addressed in f30b8e84d: - `encode_bytes_to_slice` now checks the output length unconditionally and returns an internal error, instead of the `debug_assert` that let release builds silently encode only part of the input. Good catch — the doc comment I had written described that silent truncation as if documenting it made it acceptable. - Took your suggestion on the `write_digits` comment. Leaving as follow-ons, as you suggested: - `encode_bytes` returning an owned `String`, and `md5` building those strings before copying into the `StringViewArray`. Both allocate more than they need to, and both predate this PR — worth a separate change that reworks the callers onto a reusable buffer and a `StringViewBuilder` rather than papering over it here. - Moving `to_hex`'s scalar path into the common crate. Happy to file issues for those if you'd like them tracked. -- 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]
