dalingmeng commented on code in PR #263:
URL: https://github.com/apache/paimon-cpp/pull/263#discussion_r3913490397
##########
src/paimon/common/utils/string_utils.cpp:
##########
@@ -85,18 +93,54 @@ bool StringUtils::EndsWith(const std::string& str, const
std::string& suffix) {
size_t s2 = suffix.size();
return (s1 >= s2) && (str.compare(s1 - s2, s2, suffix) == 0);
}
-bool StringUtils::IsNullOrWhitespaceOnly(const std::string& str) {
- if (str.empty()) {
- return true;
- }
- for (char c : str) {
- if (!std::isspace(static_cast<unsigned char>(c))) {
+
+bool StringUtils::IsBlank(std::string_view str) {
+ size_t offset = 0;
+ while (offset < str.size()) {
+ const auto first = static_cast<uint8_t>(str[offset]);
+ uint32_t code_point = 0;
+ size_t length = 0;
+ if (first <= 0x7f) {
+ code_point = first;
+ length = 1;
+ } else if (first >= 0xc2 && first <= 0xdf) {
+ code_point = first & 0x1f;
+ length = 2;
+ } else if (first >= 0xe0 && first <= 0xef) {
+ code_point = first & 0x0f;
+ length = 3;
+ } else if (first >= 0xf0 && first <= 0xf4) {
+ code_point = first & 0x07;
+ length = 4;
+ } else {
+ return false;
+ }
+ if (offset + length > str.size()) {
+ return false;
+ }
+ for (size_t i = 1; i < length; ++i) {
+ const auto continuation = static_cast<uint8_t>(str[offset + i]);
+ if ((continuation & 0xc0) != 0x80) {
+ return false;
+ }
+ code_point = (code_point << 6) | (continuation & 0x3f);
+ }
+ if ((length == 3 && code_point < 0x800) || (length == 4 && code_point
< 0x10000) ||
+ (code_point >= 0xd800 && code_point <= 0xdfff) || code_point >
0x10ffff) {
+ return false;
+ }
+ if (!IsJavaWhitespace(code_point)) {
return false;
}
+ offset += length;
}
return true;
}
+bool StringUtils::IsNullOrWhitespaceOnly(const std::string& str) {
+ return IsBlank(str);
Review Comment:
This function goes from std::isspace (ASCII only) to delegating to IsBlank
(Unicode). The direction is right — Java's InternalRowPartitionComputer:86 uses
the Unicode-aware isNullOrWhitespaceOnly, so C++ was genuinely out of line
here.But the function has 12 callers under src/, and two of them change actual
output rather than just tightening validation:
binary_row_partition_computer.cpp:158 (a blank partition value now falls into
the default partition, i.e. the partition directory changes) and
jieba_analyzer.cpp:111 (blank terms get filtered out, so index content
changes). That reaches beyond what the PR title covers, and the description
doesn't mention it.Partition paths determine where data is written, so a change
like that needs to be independently revertable — sitting in the same commit as
the listagg fix means a partition-layout problem can only be rolled back by
reverting listagg along with it. Splitting costs almost nothing: IsBlank is a
new function, listagg can use it
directly, and this PR just needs to not change that one delegation line.If
you consider the risk negligible, keeping it here is fine too — but please
state the fix in the PR description and add a Unicode-whitespace partition
value case for BinaryRowPartitionComputer.
--
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]
