goel-skd commented on code in PR #760:
URL: https://github.com/apache/iceberg-cpp/pull/760#discussion_r3488358691
##########
src/iceberg/util/string_util.h:
##########
@@ -41,29 +40,46 @@ concept FromChars = requires(const char* p, T& v) {
std::from_chars(p, p, v); };
class ICEBERG_EXPORT StringUtils {
public:
- // NOTE: These convert ASCII letters only; all other bytes, including
non-ASCII
- // (multibyte UTF-8) bytes, are passed through unchanged.
- // See https://github.com/apache/iceberg-cpp/issues/613.
- static std::string ToLower(std::string_view str) {
Review Comment:
Thanks for the review @zhjwpku. A few thoughts:
On switching those to `ToUpper`: it probably isn't a straight swap. The
case-insensitive field maps in `schema.cc`/`type.cc` are keyed by lowercase (to
match Java's `toLowerCase(Locale.ROOT)`), so flipping to upper means changing
comparison targets everywhere, and the field-name path needs unicode anyway
(`ToUpper` is ASCII-only). It also puts the "is this ASCII?" decision back on
every caller.
On `ToLowerUTF8`: we discussed the Ascii/Utf8 split earlier in this PR with
@wgtmac and landed on keeping a single `ToUpper`/`ToLower` rather than
`ToXXXAscii`/`ToXXXUtf8`.
I think we can get the speedup without splitting the API by adding an ASCII
fast-path inside `ToLower`, i.e. scan once for any byte `>= 0x80`, if the
input is pure ASCII, do the cheap ASCII lowercase and skip utf8proc, otherwise
fall back. One function, no caller changes, still unicode-correct, and the
ASCII majority gets the fast path for free.
Since it's a perf optimization and not correctness, I'd suggest doing it as
a small follow-up so it doesn't hold up this PR. WDYT?
--
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]
