From a80b3ae66e22a8691e72074b1d72aea4bad1d096 Mon Sep 17 00:00:00 2001 From: lxy264173 Date: Wed, 26 Aug 2026 17:51:13 +0800 Subject: [PATCH 1/4] refactor(utils): consolidate string and option handling --- .../data/variant/variant_access_utils.cpp | 3 +- .../common/types/data_type_json_parser.cpp | 6 +- src/paimon/common/utils/options_utils.h | 22 ++++++- .../common/utils/options_utils_test.cpp | 19 ++++++- src/paimon/common/utils/string_utils.cpp | 57 ++++++++++++++++--- src/paimon/common/utils/string_utils.h | 31 ++++------ src/paimon/common/utils/string_utils_test.cpp | 32 +++++++++++ .../lookup_merge_tree_compact_rewriter.cpp | 11 +--- .../compact/merge_tree_compact_rewriter.cpp | 11 +--- src/paimon/core/mergetree/lookup_levels.cpp | 11 +--- .../append_only_file_store_write.cpp | 8 +-- .../commit/sequence_snapshot_properties.cpp | 18 ++---- .../sequence_snapshot_properties_test.cpp | 2 +- .../core/postpone/postpone_bucket_writer.cpp | 18 +----- src/paimon/core/schema/schema_validation.cpp | 2 +- .../core/schema/schema_validation_test.cpp | 6 ++ .../table/system/global_system_tables.cpp | 7 ++- .../format/parquet/parquet_format_defs.h | 2 +- src/paimon/fs/local/local_file.cpp | 2 +- src/paimon/fs/local/local_file_test.cpp | 5 ++ src/paimon/fs/s3/s3_file_system.cpp | 12 ++-- .../global_index/lucene/jieba_analyzer.cpp | 6 +- src/paimon/rest/dlf_auth.cpp | 25 +------- src/paimon/rest/rest_api.cpp | 7 ++- src/paimon/rest/rest_auth.cpp | 15 +++-- src/paimon/rest/rest_catalog.cpp | 2 +- src/paimon/rest/rest_http_client.cpp | 3 +- src/paimon/rest/rest_util.cpp | 9 +-- 28 files changed, 198 insertions(+), 154 deletions(-) diff --git a/src/paimon/common/data/variant/variant_access_utils.cpp b/src/paimon/common/data/variant/variant_access_utils.cpp index 78180d1ce..0238c086a 100644 --- a/src/paimon/common/data/variant/variant_access_utils.cpp +++ b/src/paimon/common/data/variant/variant_access_utils.cpp @@ -26,6 +26,7 @@ #include "fmt/format.h" #include "paimon/common/data/variant/variant_defs.h" #include "paimon/common/types/data_field.h" +#include "paimon/common/utils/string_utils.h" namespace paimon { @@ -61,7 +62,7 @@ std::vector SplitDescription(const std::string& description) { } bool HasAccessDescription(const std::shared_ptr& field) { - return GetDescription(field).rfind(VariantAccessUtils::kMetadataKey, 0) == 0; + return StringUtils::StartsWith(GetDescription(field), VariantAccessUtils::kMetadataKey); } } // namespace diff --git a/src/paimon/common/types/data_type_json_parser.cpp b/src/paimon/common/types/data_type_json_parser.cpp index e95582a17..d04e5ade5 100644 --- a/src/paimon/common/types/data_type_json_parser.cpp +++ b/src/paimon/common/types/data_type_json_parser.cpp @@ -19,7 +19,6 @@ #include "paimon/common/types/data_type_json_parser.h" -#include #include #include #include @@ -331,10 +330,7 @@ std::vector Tokenize(const std::string& chars) { builder.clear(); cursor = ConsumeIdentifier(chars, cursor, builder); auto token = builder.str(); - auto normalized_token = token; - std::transform(normalized_token.begin(), normalized_token.end(), - normalized_token.begin(), - [](unsigned char c) { return std::toupper(c); }); + std::string normalized_token = StringUtils::ToUpperCase(token); if (Keywords().find(normalized_token) != Keywords().end()) { tokens.emplace_back(TokenType::KEYWORD, cursor, normalized_token); } else { diff --git a/src/paimon/common/utils/options_utils.h b/src/paimon/common/utils/options_utils.h index c20140071..4c93451ad 100644 --- a/src/paimon/common/utils/options_utils.h +++ b/src/paimon/common/utils/options_utils.h @@ -89,13 +89,29 @@ class OptionsUtils { return value.status(); } + static Result GetNonEmptyValueFromMap( + const std::map& key_value_map, const std::string& key) { + Result value = GetValueFromMap(key_value_map, key); + if (!value.ok()) { + return value.status(); + } + if (value.value().empty()) { + return Status::Invalid(fmt::format("value for key {} must not be empty", key)); + } + return value.value(); + } + /// Fetch options with specific prefix and remove prefix for key. + /// + /// If `ignore_empty_key` is true, an option whose key equals `prefix` is ignored. static std::map FetchOptionsWithPrefix( - const std::string& prefix, const std::map& options) { + const std::string& prefix, const std::map& options, + bool ignore_empty_key = false) { std::map options_with_prefix; - int64_t prefix_len = prefix.size(); + const std::string::size_type prefix_len = prefix.size(); for (const auto& [key, value] : options) { - if (StringUtils::StartsWith(key, prefix)) { + if (StringUtils::StartsWith(key, prefix) && + (!ignore_empty_key || key.size() > prefix_len)) { options_with_prefix[key.substr(prefix_len)] = value; } } diff --git a/src/paimon/common/utils/options_utils_test.cpp b/src/paimon/common/utils/options_utils_test.cpp index d4641184f..7eb66f65a 100644 --- a/src/paimon/common/utils/options_utils_test.cpp +++ b/src/paimon/common/utils/options_utils_test.cpp @@ -84,9 +84,24 @@ TEST(OptionsUtilsTest, TestGetOptionalValueFromMap) { } TEST(OptionsUtilsTest, TestFetchOptionsWithPrefix) { - std::map options = {{"key1", "value1"}, {"test.key2", "value2"}}; + std::map options = { + {"key1", "value1"}, {"test.", "empty-key"}, {"test.key2", "value2"}}; auto new_options = OptionsUtils::FetchOptionsWithPrefix("test.", options); - std::map expected = {{"key2", "value2"}}; + std::map expected = {{"", "empty-key"}, {"key2", "value2"}}; ASSERT_EQ(expected, new_options); + + new_options = OptionsUtils::FetchOptionsWithPrefix("test.", options, + /*ignore_empty_key=*/true); + expected = {{"key2", "value2"}}; + ASSERT_EQ(expected, new_options); +} + +TEST(OptionsUtilsTest, TestGetNonEmptyValueFromMap) { + std::map options = {{"present", "value"}, {"empty", ""}}; + ASSERT_OK_AND_ASSIGN(std::string value, + OptionsUtils::GetNonEmptyValueFromMap(options, "present")); + ASSERT_EQ("value", value); + ASSERT_TRUE(OptionsUtils::GetNonEmptyValueFromMap(options, "missing").status().IsNotExist()); + ASSERT_TRUE(OptionsUtils::GetNonEmptyValueFromMap(options, "empty").status().IsInvalid()); } } // namespace paimon::test diff --git a/src/paimon/common/utils/string_utils.cpp b/src/paimon/common/utils/string_utils.cpp index 5b405895d..869e184d0 100644 --- a/src/paimon/common/utils/string_utils.cpp +++ b/src/paimon/common/utils/string_utils.cpp @@ -30,8 +30,28 @@ #include "paimon/status.h" namespace paimon { +namespace { + +bool IsTrimCharacter(unsigned char c) { + // Match the characters removed by Java String::trim for the ASCII strings handled here. + return c <= 0x20; +} + +char ToAsciiLower(unsigned char c) { + return c >= 'A' && c <= 'Z' ? static_cast(c + ('a' - 'A')) : static_cast(c); +} + +char ToAsciiUpper(unsigned char c) { + return c >= 'a' && c <= 'z' ? static_cast(c - ('a' - 'A')) : static_cast(c); +} + +} // namespace + std::string StringUtils::Replace(const std::string& text, const std::string& search_string, const std::string& replacement, int32_t max) { + if (text.empty() || search_string.empty() || max == 0) { + return text; + } std::string str = text; size_t pos = str.find(search_string); int32_t count = 0; @@ -45,6 +65,9 @@ std::string StringUtils::Replace(const std::string& text, const std::string& sea std::string StringUtils::ReplaceLast(const std::string& text, const std::string& old_str, const std::string& new_str) { + if (text.empty() || old_str.empty()) { + return text; + } std::string str = text; size_t pos = str.rfind(old_str); if (pos != std::string::npos) { @@ -54,7 +77,8 @@ std::string StringUtils::ReplaceLast(const std::string& text, const std::string& } bool StringUtils::StartsWith(const std::string& str, const std::string& prefix, size_t start_pos) { - return (str.size() >= prefix.size()) && (str.compare(start_pos, prefix.size(), prefix) == 0); + return start_pos <= str.size() && prefix.size() <= str.size() - start_pos && + str.compare(start_pos, prefix.size(), prefix) == 0; } bool StringUtils::EndsWith(const std::string& str, const std::string& suffix) { size_t s1 = str.size(); @@ -74,26 +98,45 @@ bool StringUtils::IsNullOrWhitespaceOnly(const std::string& str) { } void StringUtils::Trim(std::string* str) { - str->erase(str->find_last_not_of(' ') + 1); - str->erase(0, str->find_first_not_of(' ')); + auto first = std::find_if_not(str->begin(), str->end(), + [](unsigned char c) { return IsTrimCharacter(c); }); + auto last = std::find_if_not(str->rbegin(), str->rend(), [](unsigned char c) { + return IsTrimCharacter(c); + }).base(); + if (first >= last) { + str->clear(); + return; + } + *str = std::string(first, last); } std::string StringUtils::ToLowerCase(const std::string& str) { std::string result; result.reserve(str.length()); - std::transform(str.begin(), str.end(), std::back_inserter(result), - [](unsigned char c) { return std::tolower(c); }); + std::transform(str.begin(), str.end(), std::back_inserter(result), ToAsciiLower); return result; } std::string StringUtils::ToUpperCase(const std::string& str) { std::string result; result.reserve(str.length()); - std::transform(str.begin(), str.end(), std::back_inserter(result), - [](unsigned char c) { return std::toupper(c); }); + std::transform(str.begin(), str.end(), std::back_inserter(result), ToAsciiUpper); return result; } +bool StringUtils::EqualsIgnoreCase(const std::string& left, const std::string& right) { + if (left.size() != right.size()) { + return false; + } + for (size_t i = 0; i < left.size(); ++i) { + if (ToAsciiLower(static_cast(left[i])) != + ToAsciiLower(static_cast(right[i]))) { + return false; + } + } + return true; +} + std::vector StringUtils::Split(const std::string& text, const std::string& sep_str, bool ignore_empty) { std::vector vec; diff --git a/src/paimon/common/utils/string_utils.h b/src/paimon/common/utils/string_utils.h index 3c0906e2e..7681a8d9e 100644 --- a/src/paimon/common/utils/string_utils.h +++ b/src/paimon/common/utils/string_utils.h @@ -50,24 +50,18 @@ class PAIMON_EXPORT StringUtils { public: /// Replaces all occurrences of a string within another string. /// - /// A `null` reference passed to this method is a no-op. - /// /// 
-    /// StringUtils::Replace(null, *, *)        = null
     /// StringUtils::Replace("", *, *)          = ""
-    /// StringUtils::Replace("any", null, *)    = "any"
-    /// StringUtils::Replace("any", *, null)    = "any"
     /// StringUtils::Replace("any", "", *)      = "any"
-    /// StringUtils::Replace("aba", "a", null)  = "aba"
     /// StringUtils::Replace("aba", "a", "")    = "b"
     /// StringUtils::Replace("aba", "a", "z")   = "zbz"
     ///
/// /// @see #replace(string text, string search_string, string replacement, int max) - /// @param text text to search and replace in, may be null - /// @param search_string the String to search for, may be null - /// @param replacement the String to replace it with, may be null - /// @return the text with any replacements processed, `null` if null string input + /// @param text text to search and replace in + /// @param search_string the String to search for + /// @param replacement the String to replace it with + /// @return the text with any replacements processed static std::string Replace(const std::string& text, const std::string& search_string, const std::string& replacement) { return Replace(text, search_string, replacement, -1); @@ -76,16 +70,10 @@ class PAIMON_EXPORT StringUtils { /// Replaces a String with another String inside a larger String, for the first `max` values of /// the search String. /// - /// A `null` reference passed to this method is a no-op. - /// /// 
-    /// StringUtils::Replace(null, *, *, *)         = null
     /// StringUtils::Replace("", *, *, *)           = ""
-    /// StringUtils::Replace("any", null, *, *)     = "any"
-    /// StringUtils::Replace("any", *, null, *)     = "any"
     /// StringUtils::Replace("any", "", *, *)       = "any"
     /// StringUtils::Replace("any", *, *, 0)        = "any"
-    /// StringUtils::Replace("abaa", "a", null, -1) = "abaa"
     /// StringUtils::Replace("abaa", "a", "", -1)   = "b"
     /// StringUtils::Replace("abaa", "a", "z", 0)   = "abaa"
     /// StringUtils::Replace("abaa", "a", "z", 1)   = "zbaa"
@@ -93,11 +81,11 @@ class PAIMON_EXPORT StringUtils {
     /// StringUtils::Replace("abaa", "a", "z", -1)  = "zbzz"
     ///
/// - /// @param text text to search and replace in, may be null - /// @param search_string the String to search for, may be null - /// @param replacement the String to replace it with, may be null + /// @param text text to search and replace in + /// @param search_string the String to search for + /// @param replacement the String to replace it with /// @param max maximum number of values to replace, or `-1` if no maximum - /// @return the text with any replacements processed, `null` if null string input + /// @return the text with any replacements processed static std::string Replace(const std::string& text, const std::string& search_string, const std::string& replacement, int32_t max); @@ -115,6 +103,9 @@ class PAIMON_EXPORT StringUtils { static std::string ToLowerCase(const std::string& str); static std::string ToUpperCase(const std::string& str); + /// Compares two strings using ASCII case folding. + static bool EqualsIgnoreCase(const std::string& left, const std::string& right); + template static std::string VectorToString(const std::vector& vec) { std::vector strs; diff --git a/src/paimon/common/utils/string_utils_test.cpp b/src/paimon/common/utils/string_utils_test.cpp index 11c3e0005..451c90357 100644 --- a/src/paimon/common/utils/string_utils_test.cpp +++ b/src/paimon/common/utils/string_utils_test.cpp @@ -73,6 +73,8 @@ void StringUtilsTest::CheckOverFlowAndUnderFlow(const std::string& over_flow, } TEST_F(StringUtilsTest, TestReplaceAll) { + ASSERT_EQ("abc", StringUtils::Replace("abc", "", "x")); + ASSERT_EQ("", StringUtils::Replace("", "a", "b")); { std::string origin = "how is is you"; std::string expect = "how are are you"; @@ -118,6 +120,8 @@ TEST_F(StringUtilsTest, TestReplaceAll) { } TEST_F(StringUtilsTest, TestReplaceLast) { + ASSERT_EQ("abc", StringUtils::ReplaceLast("abc", "", "x")); + ASSERT_EQ("", StringUtils::ReplaceLast("", "a", "b")); { std::string origin = "a/b/c//"; std::string expect = "a/b/c/_"; @@ -140,6 +144,7 @@ TEST_F(StringUtilsTest, TestReplaceLast) { } TEST_F(StringUtilsTest, TestReplaceWithMaxCount) { + ASSERT_EQ("abc", StringUtils::Replace("abc", "a", "b", 0)); { std::string origin = "how is is you"; std::string expect = "how are is you"; @@ -236,6 +241,13 @@ TEST_F(StringUtilsTest, TestToUpperCase) { } } +TEST_F(StringUtilsTest, TestEqualsIgnoreCase) { + ASSERT_TRUE(StringUtils::EqualsIgnoreCase("", "")); + ASSERT_TRUE(StringUtils::EqualsIgnoreCase("AbC-123", "aBc-123")); + ASSERT_FALSE(StringUtils::EqualsIgnoreCase("abc", "abcd")); + ASSERT_FALSE(StringUtils::EqualsIgnoreCase("abc", "abD")); +} + TEST_F(StringUtilsTest, TestStartsWith) { { std::string str = "abcde"; @@ -261,6 +273,26 @@ TEST_F(StringUtilsTest, TestStartsWith) { std::string str = ""; ASSERT_TRUE(StringUtils::StartsWith(str, "")); } + { + std::string str = "abc"; + ASSERT_TRUE(StringUtils::StartsWith(str, "", /*start_pos=*/3)); + ASSERT_FALSE(StringUtils::StartsWith(str, "", /*start_pos=*/4)); + ASSERT_FALSE(StringUtils::StartsWith(str, "a", /*start_pos=*/4)); + } +} + +TEST_F(StringUtilsTest, TestTrim) { + std::string value = " \tabc\r\n"; + StringUtils::Trim(&value); + ASSERT_EQ("abc", value); + + value = "\t\r\n"; + StringUtils::Trim(&value); + ASSERT_TRUE(value.empty()); + + value.clear(); + StringUtils::Trim(&value); + ASSERT_TRUE(value.empty()); } TEST_F(StringUtilsTest, TestEndsWith) { { diff --git a/src/paimon/core/mergetree/compact/lookup_merge_tree_compact_rewriter.cpp b/src/paimon/core/mergetree/compact/lookup_merge_tree_compact_rewriter.cpp index 994b0e3fc..071456e48 100644 --- a/src/paimon/core/mergetree/compact/lookup_merge_tree_compact_rewriter.cpp +++ b/src/paimon/core/mergetree/compact/lookup_merge_tree_compact_rewriter.cpp @@ -78,14 +78,9 @@ LookupMergeTreeCompactRewriter::Create( .WithMemoryPool(pool); PAIMON_ASSIGN_OR_RAISE(std::shared_ptr read_context, read_context_builder.Finish()); - // TODO(xinyu.lxy): temporarily disabled pre-buffer for parquet, which may cause high memory - // usage during compaction. Will fix via parquet format refactor. - auto new_options = options.ToMap(); - if (new_options.find("parquet.read.enable-pre-buffer") == new_options.end()) { - new_options["parquet.read.enable-pre-buffer"] = "false"; - } - PAIMON_ASSIGN_OR_RAISE(std::shared_ptr internal_context, - InternalReadContext::Create(read_context, table_schema, new_options)); + PAIMON_ASSIGN_OR_RAISE( + std::shared_ptr internal_context, + InternalReadContext::Create(read_context, table_schema, options.ToMap())); PAIMON_ASSIGN_OR_RAISE( std::shared_ptr path_factory, path_factory_cache->GetOrCreatePathFactory(options.GetFileFormat()->Identifier())); diff --git a/src/paimon/core/mergetree/compact/merge_tree_compact_rewriter.cpp b/src/paimon/core/mergetree/compact/merge_tree_compact_rewriter.cpp index 1b64be2da..fd7c7cbd2 100644 --- a/src/paimon/core/mergetree/compact/merge_tree_compact_rewriter.cpp +++ b/src/paimon/core/mergetree/compact/merge_tree_compact_rewriter.cpp @@ -84,14 +84,9 @@ Result> MergeTreeCompactRewriter::Crea .WithMemoryPool(pool); PAIMON_ASSIGN_OR_RAISE(std::shared_ptr read_context, read_context_builder.Finish()); - // TODO(xinyu.lxy): temporarily disabled pre-buffer for parquet, which may cause high memory - // usage during compaction. Will fix via parquet format refactor. - auto new_options = options.ToMap(); - if (new_options.find("parquet.read.enable-pre-buffer") == new_options.end()) { - new_options["parquet.read.enable-pre-buffer"] = "false"; - } - PAIMON_ASSIGN_OR_RAISE(std::shared_ptr internal_context, - InternalReadContext::Create(read_context, table_schema, new_options)); + PAIMON_ASSIGN_OR_RAISE( + std::shared_ptr internal_context, + InternalReadContext::Create(read_context, table_schema, options.ToMap())); PAIMON_ASSIGN_OR_RAISE( std::shared_ptr path_factory, path_factory_cache->GetOrCreatePathFactory(options.GetFileFormat()->Identifier())); diff --git a/src/paimon/core/mergetree/lookup_levels.cpp b/src/paimon/core/mergetree/lookup_levels.cpp index 8b5f69e77..822bd7a91 100644 --- a/src/paimon/core/mergetree/lookup_levels.cpp +++ b/src/paimon/core/mergetree/lookup_levels.cpp @@ -66,14 +66,9 @@ Result>> LookupLevels::Create( .WithMemoryPool(pool); PAIMON_ASSIGN_OR_RAISE(std::shared_ptr read_context, read_context_builder.Finish()); - // TODO(xinyu.lxy): temporarily disabled pre-buffer for parquet, which may cause high memory - // usage during compaction. Will fix via parquet format refactor. - auto new_options = options.ToMap(); - if (new_options.find("parquet.read.enable-pre-buffer") == new_options.end()) { - new_options["parquet.read.enable-pre-buffer"] = "false"; - } - PAIMON_ASSIGN_OR_RAISE(std::shared_ptr internal_read_context, - InternalReadContext::Create(read_context, table_schema, new_options)); + PAIMON_ASSIGN_OR_RAISE( + std::shared_ptr internal_read_context, + InternalReadContext::Create(read_context, table_schema, options.ToMap())); auto split_read = std::make_unique(path_factory, internal_read_context, pool, CreateDefaultExecutor()); diff --git a/src/paimon/core/operation/append_only_file_store_write.cpp b/src/paimon/core/operation/append_only_file_store_write.cpp index 5d6c2c930..f660093a2 100644 --- a/src/paimon/core/operation/append_only_file_store_write.cpp +++ b/src/paimon/core/operation/append_only_file_store_write.cpp @@ -290,14 +290,8 @@ Result> AppendOnlyFileStoreWrite::CreateFilesReader .WithMemoryPool(pool_); PAIMON_ASSIGN_OR_RAISE(std::shared_ptr read_context, context_builder.Finish()); std::map options = options_.ToMap(); - // TODO(xinyu.lxy): temporarily disabled pre-buffer for parquet, which may cause high - // memory usage during compaction. Will fix via parquet format refactor. - auto new_options = options; - if (new_options.find("parquet.read.enable-pre-buffer") == new_options.end()) { - new_options["parquet.read.enable-pre-buffer"] = "false"; - } PAIMON_ASSIGN_OR_RAISE(std::shared_ptr internal_read_context, - InternalReadContext::Create(read_context, table_schema_, new_options)); + InternalReadContext::Create(read_context, table_schema_, options)); auto read = std::make_unique(file_store_path_factory_, internal_read_context, pool_, compact_executor_); diff --git a/src/paimon/core/operation/commit/sequence_snapshot_properties.cpp b/src/paimon/core/operation/commit/sequence_snapshot_properties.cpp index 51729b991..512869f00 100644 --- a/src/paimon/core/operation/commit/sequence_snapshot_properties.cpp +++ b/src/paimon/core/operation/commit/sequence_snapshot_properties.cpp @@ -24,6 +24,7 @@ #include #include +#include "paimon/common/utils/string_utils.h" #include "paimon/core/manifest/file_kind.h" namespace paimon { @@ -40,19 +41,12 @@ Result> SequenceSnapshotProperties::MaxSequenceNumber( return std::optional(); } - try { - size_t parsed = 0; - int64_t value = std::stoll(iter->second, &parsed); - if (parsed != iter->second.size()) { - return Status::Invalid( - fmt::format("Invalid {} value '{}': trailing characters are not allowed", - kMaxSequenceNumberKey, iter->second)); - } - return std::optional(value); - } catch (const std::exception& e) { - return Status::Invalid(fmt::format("Invalid {} value '{}': {}", kMaxSequenceNumberKey, - iter->second, e.what())); + std::optional value = StringUtils::StringToValue(iter->second); + if (!value) { + return Status::Invalid( + fmt::format("Invalid {} value '{}'", kMaxSequenceNumberKey, iter->second)); } + return value; } std::optional SequenceSnapshotProperties::MaxSequenceNumberFromFiles( diff --git a/src/paimon/core/operation/commit/sequence_snapshot_properties_test.cpp b/src/paimon/core/operation/commit/sequence_snapshot_properties_test.cpp index af572b720..f8b5c1b55 100644 --- a/src/paimon/core/operation/commit/sequence_snapshot_properties_test.cpp +++ b/src/paimon/core/operation/commit/sequence_snapshot_properties_test.cpp @@ -115,7 +115,7 @@ TEST_F(SequenceSnapshotPropertiesTest, MaxSequenceNumberTrailingCharacters) { std::map properties{ {SequenceSnapshotProperties::kMaxSequenceNumberKey, "123abc"}}; ASSERT_NOK_WITH_MSG(SequenceSnapshotProperties::MaxSequenceNumber(MakeSnapshot(properties)), - "trailing characters are not allowed"); + "Invalid sequence.generation.max-sequence-number value '123abc'"); } TEST_F(SequenceSnapshotPropertiesTest, MaxSequenceNumberNotANumber) { diff --git a/src/paimon/core/postpone/postpone_bucket_writer.cpp b/src/paimon/core/postpone/postpone_bucket_writer.cpp index 47fd3bcb8..45bcfd364 100644 --- a/src/paimon/core/postpone/postpone_bucket_writer.cpp +++ b/src/paimon/core/postpone/postpone_bucket_writer.cpp @@ -34,7 +34,6 @@ #include "paimon/common/data/shredding/shredding_write_plan_factories.h" #include "paimon/common/metrics/metrics_impl.h" #include "paimon/common/table/special_fields.h" -#include "paimon/common/types/data_field.h" #include "paimon/common/types/row_kind.h" #include "paimon/common/utils/arrow/mem_utils.h" #include "paimon/common/utils/arrow/status_utils.h" @@ -55,27 +54,12 @@ namespace paimon { class InternalRow; class MemoryPool; -namespace { - -std::shared_ptr BuildPostponeBucketWriteSchema( - const std::shared_ptr& value_schema) { - arrow::FieldVector target_fields; - target_fields.push_back( - DataField::ConvertDataFieldToArrowField(SpecialFields::SequenceNumber())); - target_fields.push_back(DataField::ConvertDataFieldToArrowField(SpecialFields::ValueKind())); - target_fields.insert(target_fields.end(), value_schema->fields().begin(), - value_schema->fields().end()); - return arrow::schema(target_fields); -} - -} // namespace - Result> PostponeBucketWriter::Create( const std::vector& trimmed_primary_keys, const std::shared_ptr& path_factory, int64_t schema_id, const std::shared_ptr& value_schema, const CoreOptions& options, const std::shared_ptr& pool) { - auto write_schema = BuildPostponeBucketWriteSchema(value_schema); + auto write_schema = SpecialFields::CompleteSequenceAndValueKindField(value_schema); return std::unique_ptr(new PostponeBucketWriter( trimmed_primary_keys, path_factory, schema_id, value_schema, write_schema, options, pool)); } diff --git a/src/paimon/core/schema/schema_validation.cpp b/src/paimon/core/schema/schema_validation.cpp index 7342826d4..4c7dd2ce5 100644 --- a/src/paimon/core/schema/schema_validation.cpp +++ b/src/paimon/core/schema/schema_validation.cpp @@ -100,7 +100,7 @@ Status ValidateSharedShreddingFileFormat(const std::string& option_key, } Status ValidateVectorFileFormat(const std::string& option_key, const std::string& file_format) { - if (StringUtils::ToLowerCase(file_format) != "parquet") { + if (!StringUtils::EqualsIgnoreCase(file_format, "parquet")) { return Status::Invalid( fmt::format("VECTOR currently only supports parquet data files, but {} is {}.", option_key, file_format)); diff --git a/src/paimon/core/schema/schema_validation_test.cpp b/src/paimon/core/schema/schema_validation_test.cpp index 47603497b..578856260 100644 --- a/src/paimon/core/schema/schema_validation_test.cpp +++ b/src/paimon/core/schema/schema_validation_test.cpp @@ -56,6 +56,12 @@ TEST(SchemaValidationTest, TestVectorType) { /*primary_keys=*/{}, parquet_options)); ASSERT_OK(SchemaValidation::ValidateTableSchema(*table_schema)); + parquet_options[Options::FILE_FORMAT] = "PARQUET"; + ASSERT_OK_AND_ASSIGN(table_schema, + TableSchema::Create(/*schema_id=*/0, schema, /*partition_keys=*/{}, + /*primary_keys=*/{}, parquet_options)); + ASSERT_OK(SchemaValidation::ValidateTableSchema(*table_schema)); + std::map orc_options = {{Options::BUCKET, "-1"}, {Options::FILE_FORMAT, "orc"}}; ASSERT_OK_AND_ASSIGN(table_schema, diff --git a/src/paimon/core/table/system/global_system_tables.cpp b/src/paimon/core/table/system/global_system_tables.cpp index dd1b4e606..6523588e8 100644 --- a/src/paimon/core/table/system/global_system_tables.cpp +++ b/src/paimon/core/table/system/global_system_tables.cpp @@ -108,11 +108,12 @@ VariantType OptionalStringValue(const std::map& option Result OptionalLongValue(const std::map& options, const std::string& key) { - if (options.find(key) == options.end()) { + PAIMON_ASSIGN_OR_RAISE(std::optional value, + OptionsUtils::GetOptionalValueFromMap(options, key)); + if (!value) { return VariantType(NullType()); } - PAIMON_ASSIGN_OR_RAISE(int64_t value, OptionsUtils::GetValueFromMap(options, key)); - return VariantType(value); + return VariantType(value.value()); } Result IsEnabled(const GlobalSystemTableRegistryEntry& entry, diff --git a/src/paimon/format/parquet/parquet_format_defs.h b/src/paimon/format/parquet/parquet_format_defs.h index 433103e54..8b205a09a 100644 --- a/src/paimon/format/parquet/parquet_format_defs.h +++ b/src/paimon/format/parquet/parquet_format_defs.h @@ -99,7 +99,7 @@ static inline const char PARQUET_READ_PREDICATE_NODE_COUNT_LIMIT[] = static inline const char PARQUET_READ_ENABLE_PAGE_INDEX_FILTER[] = "parquet.read.enable-page-index-filter"; -// Default is true. Compaction will set to false to reduce memory consumption. +// Default is true. static inline const char PARQUET_READ_ENABLE_PRE_BUFFER[] = "parquet.read.enable-pre-buffer"; static constexpr uint32_t DEFAULT_PARQUET_READ_CACHE_OPTION_PREFETCH_LIMIT = 0; diff --git a/src/paimon/fs/local/local_file.cpp b/src/paimon/fs/local/local_file.cpp index 645306676..a3f960af8 100644 --- a/src/paimon/fs/local/local_file.cpp +++ b/src/paimon/fs/local/local_file.cpp @@ -49,7 +49,7 @@ Result> LocalFile::Create(const std::string& path_str // local file system does not support path_string with scheme, e.g., "file:/tmp" will be // rewritten to "/tmp" PAIMON_ASSIGN_OR_RAISE(Path path, PathUtil::ToPath(path_string)); - if (!path.scheme.empty() && StringUtils::ToLowerCase(path.scheme) != "file") { + if (!path.scheme.empty() && !StringUtils::EqualsIgnoreCase(path.scheme, "file")) { return Status::Invalid(fmt::format("invalid scheme {} for local file system", path.scheme)); } if (path.path.empty() || path.path[0] != '/') { diff --git a/src/paimon/fs/local/local_file_test.cpp b/src/paimon/fs/local/local_file_test.cpp index f22095c90..25b9f6db9 100644 --- a/src/paimon/fs/local/local_file_test.cpp +++ b/src/paimon/fs/local/local_file_test.cpp @@ -27,6 +27,11 @@ namespace paimon::test { +TEST(LocalFileTest, TestSchemeCaseInsensitive) { + ASSERT_OK(LocalFile::Create("FILE:/tmp")); + ASSERT_NOK(LocalFile::Create("s3:/tmp")); +} + TEST(LocalFileTest, TestReadWriteEmptyContent) { auto test_root_dir = UniqueTestDirectory::Create(); ASSERT_TRUE(test_root_dir); diff --git a/src/paimon/fs/s3/s3_file_system.cpp b/src/paimon/fs/s3/s3_file_system.cpp index 49668b70f..e31cc9ca3 100644 --- a/src/paimon/fs/s3/s3_file_system.cpp +++ b/src/paimon/fs/s3/s3_file_system.cpp @@ -580,22 +580,22 @@ bool IsIpAddressAuthority(const std::string& authority) { } const char* AwsDnsSuffixForRegion(const std::string& region) { - if (region.rfind("cn-", 0) == 0) { + if (StringUtils::StartsWith(region, "cn-")) { return "amazonaws.com.cn"; } - if (region.rfind("eusc-de-", 0) == 0) { + if (StringUtils::StartsWith(region, "eusc-de-")) { return "amazonaws.eu"; } - if (region.rfind("us-iso-", 0) == 0) { + if (StringUtils::StartsWith(region, "us-iso-")) { return "c2s.ic.gov"; } - if (region.rfind("us-isob-", 0) == 0) { + if (StringUtils::StartsWith(region, "us-isob-")) { return "sc2s.sgov.gov"; } - if (region.rfind("eu-isoe-", 0) == 0) { + if (StringUtils::StartsWith(region, "eu-isoe-")) { return "cloud.adc-e.uk"; } - if (region.rfind("us-isof-", 0) == 0) { + if (StringUtils::StartsWith(region, "us-isof-")) { return "csp.hci.ic.gov"; } return "amazonaws.com"; diff --git a/src/paimon/global_index/lucene/jieba_analyzer.cpp b/src/paimon/global_index/lucene/jieba_analyzer.cpp index 39cecec2f..e0a71a81a 100644 --- a/src/paimon/global_index/lucene/jieba_analyzer.cpp +++ b/src/paimon/global_index/lucene/jieba_analyzer.cpp @@ -17,6 +17,8 @@ */ #include "paimon/global_index/lucene/jieba_analyzer.h" +#include + #include "paimon/common/utils/string_utils.h" #include "paimon/global_index/lucene/lucene_utils.h" @@ -94,9 +96,7 @@ void JiebaTokenizer::NormalizeCase(std::string* term) { } } if (is_alphanumeric && !term->empty()) { - std::transform(term->begin(), term->end(), term->begin(), [](char ch) { - return static_cast(std::tolower(static_cast(ch))); - }); + *term = StringUtils::ToLowerCase(*term); } } diff --git a/src/paimon/rest/dlf_auth.cpp b/src/paimon/rest/dlf_auth.cpp index 4c592f31f..6a4906279 100644 --- a/src/paimon/rest/dlf_auth.cpp +++ b/src/paimon/rest/dlf_auth.cpp @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -74,28 +73,10 @@ constexpr const char kAcsSignatureVersionHeader[] = "x-acs-signature-version"; constexpr const char kAcsVersionHeader[] = "x-acs-version"; constexpr const char kAcsSecurityTokenHeader[] = "x-acs-security-token"; -void TrimWhitespace(std::string* value) { - size_t begin = 0; - while (begin < value->size() && std::isspace(static_cast((*value)[begin]))) { - ++begin; - } - size_t end = value->size(); - while (end > begin && std::isspace(static_cast((*value)[end - 1]))) { - --end; - } - *value = value->substr(begin, end - begin); -} - Result RequiredNonEmptyOption(const std::map& options, const std::string& key) { - Result value = OptionsUtils::GetValueFromMap(options, key); + Result value = OptionsUtils::GetNonEmptyValueFromMap(options, key); if (!value.ok()) { - if (!value.status().IsNotExist()) { - return value.status(); - } - return Status::Invalid(fmt::format("option '{}' must be configured for DLF auth", key)); - } - if (value.value().empty()) { return Status::Invalid(fmt::format("option '{}' must be configured for DLF auth", key)); } return value.value(); @@ -267,7 +248,7 @@ Result Md5Base64(const std::string& value) { std::string Trimmed(const std::string& value) { std::string trimmed = value; - TrimWhitespace(&trimmed); + StringUtils::Trim(&trimmed); return trimmed; } @@ -510,7 +491,7 @@ Result DlfEcsTokenLoader::LoadToken() { } if (!role_name_) { PAIMON_ASSIGN_OR_RAISE(std::string role, Get(metadata_url_)); - TrimWhitespace(&role); + StringUtils::Trim(&role); if (role.empty()) { return Status::Invalid("DLF ECS metadata service returned an empty role name"); } diff --git a/src/paimon/rest/rest_api.cpp b/src/paimon/rest/rest_api.cpp index c249ede05..a28e19916 100644 --- a/src/paimon/rest/rest_api.cpp +++ b/src/paimon/rest/rest_api.cpp @@ -24,6 +24,7 @@ #include "fmt/format.h" #include "paimon/catalog_options.h" +#include "paimon/common/utils/options_utils.h" #include "paimon/common/utils/rapidjson_util.h" #include "paimon/common/utils/sensitive_config_utils.h" #include "paimon/logging.h" @@ -61,13 +62,13 @@ RestApi::RestApi(std::unique_ptr client, Result> RestApi::Create(const std::map& options, const std::string& warehouse, bool config_required, const RestHttpClient::Config& http_config) { - auto uri_iter = options.find(CatalogOptions::URI); - if (uri_iter == options.end() || uri_iter->second.empty()) { + Result uri = OptionsUtils::GetNonEmptyValueFromMap(options, CatalogOptions::URI); + if (!uri.ok()) { return Status::Invalid(fmt::format("option '{}' must be configured for the rest catalog", CatalogOptions::URI)); } PAIMON_ASSIGN_OR_RAISE(std::unique_ptr client, - RestHttpClient::Create(uri_iter->second, http_config)); + RestHttpClient::Create(uri.value(), http_config)); PAIMON_ASSIGN_OR_RAISE(std::unique_ptr auth_provider, AuthProvider::Create(options)); diff --git a/src/paimon/rest/rest_auth.cpp b/src/paimon/rest/rest_auth.cpp index 1af3b06aa..43513b701 100644 --- a/src/paimon/rest/rest_auth.cpp +++ b/src/paimon/rest/rest_auth.cpp @@ -20,6 +20,7 @@ #include "fmt/format.h" #include "paimon/catalog_options.h" +#include "paimon/common/utils/options_utils.h" #include "paimon/common/utils/string_utils.h" #include "paimon/common/utils/url_utils.h" #include "paimon/rest/dlf_auth.h" @@ -50,22 +51,24 @@ Result> BearTokenAuthProvider::MergeAuthHeade Result> AuthProvider::Create( const std::map& options) { - auto provider_iter = options.find(CatalogOptions::TOKEN_PROVIDER); - if (provider_iter == options.end() || provider_iter->second.empty()) { + Result provider_value = + OptionsUtils::GetNonEmptyValueFromMap(options, CatalogOptions::TOKEN_PROVIDER); + if (!provider_value.ok()) { return Status::Invalid(fmt::format("option '{}' must be configured for the rest catalog", CatalogOptions::TOKEN_PROVIDER)); } // Matched leniently in lower case; other clients may match provider names // case-sensitively, so the exact "bear" and "dlf" spellings are portable. - std::string provider = StringUtils::ToLowerCase(provider_iter->second); + std::string provider = StringUtils::ToLowerCase(provider_value.value()); if (provider == "bear") { - auto token_iter = options.find(CatalogOptions::TOKEN); - if (token_iter == options.end() || token_iter->second.empty()) { + Result token = + OptionsUtils::GetNonEmptyValueFromMap(options, CatalogOptions::TOKEN); + if (!token.ok()) { return Status::Invalid( fmt::format("option '{}' must be configured for the bear token provider", CatalogOptions::TOKEN)); } - return std::make_unique(token_iter->second); + return std::make_unique(token.value()); } if (provider == "dlf") { return DlfAuthProvider::Create(options); diff --git a/src/paimon/rest/rest_catalog.cpp b/src/paimon/rest/rest_catalog.cpp index c928c6a25..eb86035c5 100644 --- a/src/paimon/rest/rest_catalog.cpp +++ b/src/paimon/rest/rest_catalog.cpp @@ -52,7 +52,7 @@ constexpr const char kPathOption[] = "path"; // `BranchManager::IsMainBranch`, which names the branch directory of a table, stays // case-sensitive: this normalization only decides how a table is addressed on the server. std::optional NormalizeBranch(std::optional branch) { - if (branch && StringUtils::ToLowerCase(branch.value()) == Identifier::kDefaultMainBranch) { + if (branch && StringUtils::EqualsIgnoreCase(branch.value(), Identifier::kDefaultMainBranch)) { return std::nullopt; } return branch; diff --git a/src/paimon/rest/rest_http_client.cpp b/src/paimon/rest/rest_http_client.cpp index 99b233f1a..f4964bf57 100644 --- a/src/paimon/rest/rest_http_client.cpp +++ b/src/paimon/rest/rest_http_client.cpp @@ -228,7 +228,8 @@ std::string RestHttpClient::NormalizeUri(const std::string& uri) { while (!normalized.empty() && normalized.back() == '/') { normalized.pop_back(); } - if (normalized.rfind("http://", 0) != 0 && normalized.rfind("https://", 0) != 0) { + if (!StringUtils::StartsWith(normalized, "http://") && + !StringUtils::StartsWith(normalized, "https://")) { normalized = "http://" + normalized; } return normalized; diff --git a/src/paimon/rest/rest_util.cpp b/src/paimon/rest/rest_util.cpp index fc8e6f698..7af046791 100644 --- a/src/paimon/rest/rest_util.cpp +++ b/src/paimon/rest/rest_util.cpp @@ -21,6 +21,7 @@ #include #include "fmt/format.h" +#include "paimon/common/utils/options_utils.h" #include "rapidjson/error/en.h" #include "rapidjson/stringbuffer.h" #include "rapidjson/writer.h" @@ -29,13 +30,7 @@ namespace paimon { std::map RestUtil::ExtractPrefixMap( const std::map& options, const std::string& prefix) { - std::map result; - for (const auto& [key, value] : options) { - if (key.size() > prefix.size() && key.compare(0, prefix.size(), prefix) == 0) { - result[key.substr(prefix.size())] = value; - } - } - return result; + return OptionsUtils::FetchOptionsWithPrefix(prefix, options, /*ignore_empty_key=*/true); } std::string RestUtil::ExtractRequestId(const std::map& headers) { From 8ac8ab4d54beebcbfef503eae2a751c1d7b2fb42 Mon Sep 17 00:00:00 2001 From: lxy264173 Date: Wed, 26 Aug 2026 18:04:52 +0800 Subject: [PATCH 2/4] fix pre-commit --- src/paimon/common/utils/string_utils_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/paimon/common/utils/string_utils_test.cpp b/src/paimon/common/utils/string_utils_test.cpp index 451c90357..a4f230781 100644 --- a/src/paimon/common/utils/string_utils_test.cpp +++ b/src/paimon/common/utils/string_utils_test.cpp @@ -245,7 +245,7 @@ TEST_F(StringUtilsTest, TestEqualsIgnoreCase) { ASSERT_TRUE(StringUtils::EqualsIgnoreCase("", "")); ASSERT_TRUE(StringUtils::EqualsIgnoreCase("AbC-123", "aBc-123")); ASSERT_FALSE(StringUtils::EqualsIgnoreCase("abc", "abcd")); - ASSERT_FALSE(StringUtils::EqualsIgnoreCase("abc", "abD")); + ASSERT_FALSE(StringUtils::EqualsIgnoreCase("abc", "abx")); } TEST_F(StringUtilsTest, TestStartsWith) { From 5ea8178d749b0314c11eeebb140562281288efe9 Mon Sep 17 00:00:00 2001 From: lxy264173 Date: Wed, 26 Aug 2026 22:04:58 +0800 Subject: [PATCH 3/4] refactor CoreOptions --- src/paimon/core/core_options.cpp | 281 +++++++++++++------------------ 1 file changed, 116 insertions(+), 165 deletions(-) diff --git a/src/paimon/core/core_options.cpp b/src/paimon/core/core_options.cpp index 6320ec577..a5b32b09a 100644 --- a/src/paimon/core/core_options.cpp +++ b/src/paimon/core/core_options.cpp @@ -19,6 +19,7 @@ #include "paimon/core/core_options.h" #include +#include #include #include #include @@ -51,14 +52,9 @@ class ConfigParser { // Parse basic type configurations template Status Parse(const std::string& key, T* value) const { - auto iter = config_map_.find(key); - if (iter != config_map_.end()) { - auto result = StringUtils::StringToValue(iter->second); - if (result) { - *value = result.value(); - return Status::OK(); - } - return Status::Invalid(fmt::format("Invalid Config [{}: {}]", key, iter->second)); + PAIMON_ASSIGN_OR_RAISE(std::optional parsed_value, GetOptionalValue(key)); + if (parsed_value) { + *value = parsed_value.value(); } return Status::OK(); // Return success even if the configuration does not exist } @@ -66,14 +62,9 @@ class ConfigParser { // Parse optional basic type configurations template Status Parse(const std::string& key, std::optional* value) const { - auto iter = config_map_.find(key); - if (iter != config_map_.end()) { - auto result = StringUtils::StringToValue(iter->second); - if (result) { - *value = result.value(); - return Status::OK(); - } - return Status::Invalid(fmt::format("Invalid Config [{}: {}]", key, iter->second)); + PAIMON_ASSIGN_OR_RAISE(std::optional parsed_value, GetOptionalValue(key)); + if (parsed_value) { + *value = parsed_value.value(); } return Status::OK(); // Return success even if the configuration does not exist } @@ -82,23 +73,26 @@ class ConfigParser { template Status ParseList(const std::string& key, const std::string& delimiter, std::vector* list, bool need_trim = false) const { - auto iter = config_map_.find(key); - if (iter != config_map_.end()) { - auto value_str_vec = StringUtils::Split(iter->second, delimiter, /*ignore_empty=*/true); - for (auto& value_str : value_str_vec) { - if (need_trim) { - StringUtils::Trim(&value_str); - } - if constexpr (std::is_same_v) { - list->emplace_back(value_str); - } else { - auto value = StringUtils::StringToValue(value_str); - if (!value) { - return Status::Invalid( - fmt::format("Invalid Config [{}: {}]", key, iter->second)); - } - list->emplace_back(value.value()); + PAIMON_ASSIGN_OR_RAISE(std::optional config_value, + GetOptionalValue(key)); + if (!config_value) { + return Status::OK(); + } + auto value_str_vec = + StringUtils::Split(config_value.value(), delimiter, /*ignore_empty=*/true); + for (auto& value_str : value_str_vec) { + if (need_trim) { + StringUtils::Trim(&value_str); + } + if constexpr (std::is_same_v) { + list->emplace_back(value_str); + } else { + auto value = StringUtils::StringToValue(value_str); + if (!value) { + return Status::Invalid( + fmt::format("Invalid Config [{}: {}]", key, config_value.value())); } + list->emplace_back(value.value()); } } return Status::OK(); // Return success even if the configuration does not exist @@ -109,9 +103,10 @@ class ConfigParser { Status ParseMemorySize(const std::string& key, T* value) const { static_assert(std::is_same_v || std::is_same_v>, "ParseMemorySize only supports int64_t and std::optional"); - auto iter = config_map_.find(key); - if (iter != config_map_.end()) { - PAIMON_ASSIGN_OR_RAISE(*value, MemorySize::ParseBytes(iter->second)); + PAIMON_ASSIGN_OR_RAISE(std::optional config_value, + GetOptionalValue(key)); + if (config_value) { + PAIMON_ASSIGN_OR_RAISE(*value, MemorySize::ParseBytes(config_value.value())); } return Status::OK(); } @@ -121,9 +116,10 @@ class ConfigParser { Status ParseTimeDuration(const std::string& key, T* value) const { static_assert(std::is_same_v || std::is_same_v>, "ParseTimeDuration only supports int64_t and std::optional"); - auto iter = config_map_.find(key); - if (iter != config_map_.end()) { - PAIMON_ASSIGN_OR_RAISE(*value, TimeDuration::Parse(iter->second)); + PAIMON_ASSIGN_OR_RAISE(std::optional config_value, + GetOptionalValue(key)); + if (config_value) { + PAIMON_ASSIGN_OR_RAISE(*value, TimeDuration::Parse(config_value.value())); } return Status::OK(); } @@ -132,14 +128,10 @@ class ConfigParser { template Status ParseObject(const std::string& key, const std::string& default_identifier, std::shared_ptr* value) const { - auto iter = config_map_.find(key); - if (iter != config_map_.end()) { - std::string normalized_value = StringUtils::ToLowerCase(iter->second); - PAIMON_ASSIGN_OR_RAISE(*value, Factory::Get(normalized_value, config_map_)); - } else { - PAIMON_ASSIGN_OR_RAISE( - *value, Factory::Get(StringUtils::ToLowerCase(default_identifier), config_map_)); - } + PAIMON_ASSIGN_OR_RAISE(std::string identifier, OptionsUtils::GetValueFromMap( + config_map_, key, default_identifier)); + PAIMON_ASSIGN_OR_RAISE(*value, + Factory::Get(StringUtils::ToLowerCase(identifier), config_map_)); return Status::OK(); } @@ -152,11 +144,10 @@ class ConfigParser { *value = specified_file_system; return Status::OK(); } - std::string default_fs_identifier = "local"; - auto iter = config_map_.find(Options::FILE_SYSTEM); - if (iter != config_map_.end()) { - default_fs_identifier = StringUtils::ToLowerCase(iter->second); - } + PAIMON_ASSIGN_OR_RAISE( + std::string default_fs_identifier, + OptionsUtils::GetValueFromMap(config_map_, Options::FILE_SYSTEM, "local")); + default_fs_identifier = StringUtils::ToLowerCase(default_fs_identifier); *value = std::make_shared(fs_scheme_to_identifier_map, default_fs_identifier, config_map_); return Status::OK(); @@ -164,151 +155,81 @@ class ConfigParser { // Parse SortOrder Status ParseSortOrder(SortOrder* sort_order) const { - auto iter = config_map_.find(Options::SEQUENCE_FIELD_SORT_ORDER); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "ascending") { - *sort_order = SortOrder::ASCENDING; - } else if (str == "descending") { - *sort_order = SortOrder::DESCENDING; - } else { - return Status::Invalid(fmt::format("invalid sort order: {}", str)); - } - } - return Status::OK(); + return ParseEnum( + Options::SEQUENCE_FIELD_SORT_ORDER, + {{"ascending", SortOrder::ASCENDING}, {"descending", SortOrder::DESCENDING}}, + "sort order", sort_order); } // Parse LookupCompactMode Status ParseLookupCompactMode(LookupCompactMode* mode) const { - auto iter = config_map_.find(Options::LOOKUP_COMPACT); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "radical") { - *mode = LookupCompactMode::RADICAL; - } else if (str == "gentle") { - *mode = LookupCompactMode::GENTLE; - } else { - return Status::Invalid(fmt::format("invalid lookup mode: {}", str)); - } - } - return Status::OK(); + return ParseEnum( + Options::LOOKUP_COMPACT, + {{"radical", LookupCompactMode::RADICAL}, {"gentle", LookupCompactMode::GENTLE}}, + "lookup mode", mode); } // Parse SortEngine Status ParseSortEngine(SortEngine* sort_engine) const { - auto iter = config_map_.find(Options::SORT_ENGINE); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "min-heap") { - *sort_engine = SortEngine::MIN_HEAP; - } else if (str == "loser-tree") { - *sort_engine = SortEngine::LOSER_TREE; - } else { - return Status::Invalid(fmt::format("invalid sort engine: {}", str)); - } - } - return Status::OK(); + return ParseEnum( + Options::SORT_ENGINE, + {{"min-heap", SortEngine::MIN_HEAP}, {"loser-tree", SortEngine::LOSER_TREE}}, + "sort engine", sort_engine); } // Parse MergeEngine Status ParseMergeEngine(MergeEngine* merge_engine) const { - auto iter = config_map_.find(Options::MERGE_ENGINE); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "deduplicate") { - *merge_engine = MergeEngine::DEDUPLICATE; - } else if (str == "partial-update") { - *merge_engine = MergeEngine::PARTIAL_UPDATE; - } else if (str == "aggregation") { - *merge_engine = MergeEngine::AGGREGATE; - } else if (str == "first-row") { - *merge_engine = MergeEngine::FIRST_ROW; - } else { - return Status::Invalid(fmt::format("invalid merge engine: {}", str)); - } - } - return Status::OK(); + return ParseEnum(Options::MERGE_ENGINE, + {{"deduplicate", MergeEngine::DEDUPLICATE}, + {"partial-update", MergeEngine::PARTIAL_UPDATE}, + {"aggregation", MergeEngine::AGGREGATE}, + {"first-row", MergeEngine::FIRST_ROW}}, + "merge engine", merge_engine); } // Parse VariantShreddingInferenceMode Status ParseVariantShreddingInferenceMode(VariantShreddingInferenceMode* inference_mode) const { - auto iter = config_map_.find(Options::VARIANT_SHREDDING_INFERENCE_MODE); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "per-file") { - *inference_mode = VariantShreddingInferenceMode::PER_FILE; - } else if (str == "adaptive") { - *inference_mode = VariantShreddingInferenceMode::ADAPTIVE; - } else { - return Status::Invalid( - fmt::format("invalid variant shredding inference mode: {}", str)); - } - } - return Status::OK(); + return ParseEnum(Options::VARIANT_SHREDDING_INFERENCE_MODE, + {{"per-file", VariantShreddingInferenceMode::PER_FILE}, + {"adaptive", VariantShreddingInferenceMode::ADAPTIVE}}, + "variant shredding inference mode", inference_mode); } // Parse ChangelogProducer Status ParseChangelogProducer(ChangelogProducer* changelog_producer) const { - auto iter = config_map_.find(Options::CHANGELOG_PRODUCER); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "none") { - *changelog_producer = ChangelogProducer::NONE; - } else if (str == "input") { - *changelog_producer = ChangelogProducer::INPUT; - } else if (str == "full-compaction") { - *changelog_producer = ChangelogProducer::FULL_COMPACTION; - } else if (str == "lookup") { - *changelog_producer = ChangelogProducer::LOOKUP; - } else { - return Status::Invalid(fmt::format("invalid changelog producer: {}", str)); - } - } - return Status::OK(); + return ParseEnum(Options::CHANGELOG_PRODUCER, + {{"none", ChangelogProducer::NONE}, + {"input", ChangelogProducer::INPUT}, + {"full-compaction", ChangelogProducer::FULL_COMPACTION}, + {"lookup", ChangelogProducer::LOOKUP}}, + "changelog producer", changelog_producer); } // Parse ExternalPathStrategy Status ParseExternalPathStrategy(ExternalPathStrategy* external_path_strategy) const { - auto iter = config_map_.find(Options::DATA_FILE_EXTERNAL_PATHS_STRATEGY); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "none") { - *external_path_strategy = ExternalPathStrategy::NONE; - } else if (str == "specific-fs") { - *external_path_strategy = ExternalPathStrategy::SPECIFIC_FS; - } else if (str == "round-robin") { - *external_path_strategy = ExternalPathStrategy::ROUND_ROBIN; - } else { - return Status::Invalid(fmt::format("invalid external path strategy: {}", str)); - } - } - return Status::OK(); + return ParseEnum(Options::DATA_FILE_EXTERNAL_PATHS_STRATEGY, + {{"none", ExternalPathStrategy::NONE}, + {"specific-fs", ExternalPathStrategy::SPECIFIC_FS}, + {"round-robin", ExternalPathStrategy::ROUND_ROBIN}}, + "external path strategy", external_path_strategy); } // Parse BucketFunctionType Status ParseBucketFunctionType(BucketFunctionType* bucket_function_type) const { - auto iter = config_map_.find(Options::BUCKET_FUNCTION_TYPE); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - if (str == "default") { - *bucket_function_type = BucketFunctionType::DEFAULT; - } else if (str == "mod") { - *bucket_function_type = BucketFunctionType::MOD; - } else if (str == "hive") { - *bucket_function_type = BucketFunctionType::HIVE; - } else { - return Status::Invalid(fmt::format("invalid bucket function type: {}", str)); - } - } - return Status::OK(); + return ParseEnum(Options::BUCKET_FUNCTION_TYPE, + {{"default", BucketFunctionType::DEFAULT}, + {"mod", BucketFunctionType::MOD}, + {"hive", BucketFunctionType::HIVE}}, + "bucket function type", bucket_function_type); } // Parse StartupMode Status ParseStartupMode(StartupMode* startup_mode) const { - auto iter = config_map_.find(Options::SCAN_MODE); - if (iter != config_map_.end()) { - std::string str = StringUtils::ToLowerCase(iter->second); - PAIMON_ASSIGN_OR_RAISE(*startup_mode, StartupMode::FromString(str)); + PAIMON_ASSIGN_OR_RAISE(std::optional value, + GetOptionalValue(Options::SCAN_MODE)); + if (value) { + PAIMON_ASSIGN_OR_RAISE( + *startup_mode, StartupMode::FromString(StringUtils::ToLowerCase(value.value()))); } return Status::OK(); } @@ -372,7 +293,37 @@ class ConfigParser { } private: - const std::map config_map_; + template + Status ParseEnum(const std::string& key, + std::initializer_list> candidates, + const std::string& error_name, T* value) const { + PAIMON_ASSIGN_OR_RAISE(std::optional config_value, + GetOptionalValue(key)); + if (!config_value) { + return Status::OK(); + } + std::string normalized_value = StringUtils::ToLowerCase(config_value.value()); + for (const auto& [candidate, candidate_value] : candidates) { + if (normalized_value == candidate) { + *value = candidate_value; + return Status::OK(); + } + } + return Status::Invalid(fmt::format("invalid {}: {}", error_name, normalized_value)); + } + + template + Result> GetOptionalValue(const std::string& key) const { + Result> result = + OptionsUtils::GetOptionalValueFromMap(config_map_, key); + if (!result.ok()) { + return Status::Invalid( + fmt::format("Invalid Config [{}: {}]", key, config_map_.at(key))); + } + return result.value(); + } + + const std::map& config_map_; }; // Impl is a private implementation of CoreOptions, From 2f611b2e1c20bdc032f5cc4b19baa0e7d7549649 Mon Sep 17 00:00:00 2001 From: lxy264173 Date: Thu, 27 Aug 2026 13:39:03 +0800 Subject: [PATCH 4/4] fix comments --- src/paimon/common/utils/options_utils.h | 12 +++++----- .../common/utils/options_utils_test.cpp | 7 +----- src/paimon/format/orc/orc_format_writer.cpp | 24 +++++-------------- src/paimon/rest/rest_util.cpp | 2 +- 4 files changed, 14 insertions(+), 31 deletions(-) diff --git a/src/paimon/common/utils/options_utils.h b/src/paimon/common/utils/options_utils.h index 4c93451ad..08b09ad2d 100644 --- a/src/paimon/common/utils/options_utils.h +++ b/src/paimon/common/utils/options_utils.h @@ -102,16 +102,16 @@ class OptionsUtils { } /// Fetch options with specific prefix and remove prefix for key. - /// - /// If `ignore_empty_key` is true, an option whose key equals `prefix` is ignored. + /// @param prefix Prefix used to select options and removed from the returned keys. + /// @param options Options to select from. + /// @return Options whose keys start with and are longer than `prefix`, with the prefix removed + /// from each key. static std::map FetchOptionsWithPrefix( - const std::string& prefix, const std::map& options, - bool ignore_empty_key = false) { + const std::string& prefix, const std::map& options) { std::map options_with_prefix; const std::string::size_type prefix_len = prefix.size(); for (const auto& [key, value] : options) { - if (StringUtils::StartsWith(key, prefix) && - (!ignore_empty_key || key.size() > prefix_len)) { + if (key.size() > prefix_len && StringUtils::StartsWith(key, prefix)) { options_with_prefix[key.substr(prefix_len)] = value; } } diff --git a/src/paimon/common/utils/options_utils_test.cpp b/src/paimon/common/utils/options_utils_test.cpp index 7eb66f65a..61e520874 100644 --- a/src/paimon/common/utils/options_utils_test.cpp +++ b/src/paimon/common/utils/options_utils_test.cpp @@ -87,12 +87,7 @@ TEST(OptionsUtilsTest, TestFetchOptionsWithPrefix) { std::map options = { {"key1", "value1"}, {"test.", "empty-key"}, {"test.key2", "value2"}}; auto new_options = OptionsUtils::FetchOptionsWithPrefix("test.", options); - std::map expected = {{"", "empty-key"}, {"key2", "value2"}}; - ASSERT_EQ(expected, new_options); - - new_options = OptionsUtils::FetchOptionsWithPrefix("test.", options, - /*ignore_empty_key=*/true); - expected = {{"key2", "value2"}}; + std::map expected = {{"key2", "value2"}}; ASSERT_EQ(expected, new_options); } diff --git a/src/paimon/format/orc/orc_format_writer.cpp b/src/paimon/format/orc/orc_format_writer.cpp index 1a394ca94..fc2316b21 100644 --- a/src/paimon/format/orc/orc_format_writer.cpp +++ b/src/paimon/format/orc/orc_format_writer.cpp @@ -40,7 +40,6 @@ #include "orc/Writer.hh" #include "paimon/common/data/variant/variant_type_utils.h" #include "paimon/common/metrics/metrics_impl.h" -#include "paimon/common/options/memory_size.h" #include "paimon/common/utils/arrow/status_utils.h" #include "paimon/common/utils/options_utils.h" #include "paimon/common/utils/string_utils.h" @@ -236,18 +235,6 @@ Status OrcFormatWriter::AddMetadata(const std::map& me return Status::OK(); } -namespace { - -Result GetMemorySizeOption(const std::map& options, - const std::string& key, uint64_t default_value) { - PAIMON_ASSIGN_OR_RAISE(std::string value, OptionsUtils::GetValueFromMap( - options, key, std::to_string(default_value))); - PAIMON_ASSIGN_OR_RAISE(int64_t bytes, MemorySize::ParseBytes(value)); - return static_cast(bytes); -} - -} // namespace - Result<::orc::WriterOptions> OrcFormatWriter::PrepareWriterOptions( const std::map& options, const std::string& file_compression, const std::shared_ptr& data_type) { @@ -261,15 +248,16 @@ Result<::orc::WriterOptions> OrcFormatWriter::PrepareWriterOptions( } } ::orc::WriterOptions writer_options; - PAIMON_ASSIGN_OR_RAISE(uint64_t stripe_size, - GetMemorySizeOption(options, ORC_STRIPE_SIZE, DEFAULT_STRIPE_SIZE)); + PAIMON_ASSIGN_OR_RAISE( + uint64_t stripe_size, + OptionsUtils::GetValueFromMap(options, ORC_STRIPE_SIZE, DEFAULT_STRIPE_SIZE)); writer_options.setStripeSize(stripe_size); PAIMON_ASSIGN_OR_RAISE(::orc::CompressionKind compression, ToOrcCompressionKind(StringUtils::ToLowerCase(file_compression))); writer_options.setCompression(compression); - PAIMON_ASSIGN_OR_RAISE( - uint64_t compression_block_size, - GetMemorySizeOption(options, ORC_COMPRESSION_BLOCK_SIZE, DEFAULT_COMPRESSION_BLOCK_SIZE)); + PAIMON_ASSIGN_OR_RAISE(uint64_t compression_block_size, OptionsUtils::GetValueFromMap( + options, ORC_COMPRESSION_BLOCK_SIZE, + DEFAULT_COMPRESSION_BLOCK_SIZE)); writer_options.setCompressionBlockSize(compression_block_size); PAIMON_ASSIGN_OR_RAISE( double dictionary_key_threshold, diff --git a/src/paimon/rest/rest_util.cpp b/src/paimon/rest/rest_util.cpp index 7af046791..1a746987d 100644 --- a/src/paimon/rest/rest_util.cpp +++ b/src/paimon/rest/rest_util.cpp @@ -30,7 +30,7 @@ namespace paimon { std::map RestUtil::ExtractPrefixMap( const std::map& options, const std::string& prefix) { - return OptionsUtils::FetchOptionsWithPrefix(prefix, options, /*ignore_empty_key=*/true); + return OptionsUtils::FetchOptionsWithPrefix(prefix, options); } std::string RestUtil::ExtractRequestId(const std::map& headers) {