feat: add StringUtils, DateTimeUtils, PathUtil, OptionsUtils, RapidJsonUtil, and Jsonizable utilities - #23
Conversation
…onUtil, and Jsonizable utilities
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for adding the utility modules. I found a blocking issue in PathUtil: NormalizePath currently turns valid URI paths without an authority, such as file:///tmp or hdfs:///warehouse, into file:/tmp / hdfs:/warehouse. In URI syntax the triple slash is significant here because it means an empty authority plus an absolute path; collapsing it changes the URI shape and can break filesystem/catalog path handling. Please preserve the scheme:///path form when the input has an explicit empty authority, and add tests for file:///.../hdfs:///... normalization.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Correction for my previous NormalizePath comment: I rechecked Apache Paimon Java Path.toString(). For new Path("file:///tmp") / new Path("hdfs:///warehouse"), the parsed authority is null and toString() emits file:/tmp / hdfs:/warehouse, not the triple-slash form. Since this C++ implementation returns Path::ToString() and matches that Java behavior, the NormalizePath URI-shape issue I raised should be ignored. No change is needed for that part.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed PR #23 after correcting my previous NormalizePath concern. The path normalization behavior is compatible with Java Paimon and does not need to change. I found two other issues that should be fixed before merging: date/timestamp parsing currently accepts impossible dates by letting libc normalize them, and JSON deserialization of narrow integer types can silently truncate out-of-range values.
There was a problem hiding this comment.
This accepts invalid calendar dates because std::get_time only fills tm, and timegm/mktime normalizes out-of-range fields instead of rejecting them. For example, StringToDate("2019-02-29") will become 2019-03-01, and StringToTimestampMillis("2023-02-30 00:00:00") will become 2023-03-02 00:00:00. Java Paimon's DateTimeUtils.parseDate explicitly validates month/day ranges and rejects invalid dates, so this can persist or compare wrong metadata values. Please validate the parsed year/month/day before converting, or compare the normalized tm back to the parsed fields, and add tests for non-leap Feb 29 / Feb 30.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
One more inline issue for the JSON integer deserialization path.
There was a problem hiding this comment.
The small-integer JSON readers only check RapidJSON's broad IsInt() / IsUint() predicates, then cast to int8_t / uint8_t / int16_t / uint16_t. This silently wraps out-of-range JSON values, e.g. deserializing 500 as int8_t yields a truncated value instead of failing. Since StringUtils::StringToValue already rejects the same overflow cases, JSON deserialization should be consistent: check value.GetInt() / GetUint() against std::numeric_limits<T>::min()/max() before casting, and add overflow tests for the narrow integer types.
Dismissed as superseded: the NormalizePath concern was incorrect after checking Java Paimon Path.toString(); no change is needed for that part.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the clarification. I agree the JSON narrow-integer overflow point is defensive under the current DeserializeKeyValue<T> / DeserializeValue<T> usage, where callers choose the concrete schema type and are expected not to feed out-of-range values for that type. It should not block this PR.
The remaining blocker is only the date/timestamp parsing issue: std::get_time plus timegm/mktime can normalize impossible calendar dates such as 2019-02-29 or 2023-02-30 instead of rejecting them. That can persist or compare a different date from the input. Please fix that validation or document why this input is impossible at the call boundary.
Dismissed after re-review: the invalid date/timestamp normalization blocker has been fixed in the latest commit.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Re-reviewed the latest commit. The previous blockers are addressed: NormalizePath matches Java Paimon Path.toString(), the JSON narrow-integer point is non-blocking under the typed deserialization contract, and invalid date/timestamp normalization is now rejected by checking the normalized month/day with tests for invalid dates. Looks good to me.
2 participants
Purpose
No Linked issue.
Introduce string/JSON/path utility modules to the apache/paimon-cpp codebase. These modules provide common string manipulation, date-time parsing, path operations, configuration option parsing, and JSON serialization helpers.
Introduce DateTimeUtils for timestamp parsing and formatting (date_time_utils.h)
Introduce TimezoneGuard test utility for timezone-dependent tests (timezone_guard.h)
Introduce StringUtils for string splitting, trimming, type conversion, and formatting (string_utils.h, string_utils.cpp)
Introduce RapidJsonUtil for JSON document read/write helpers (rapidjson_util.h)
Introduce PathUtil for file path manipulation and generation (path_util.h, path_util.cpp)
Introduce OptionsUtils for parsing configuration key-value options (options_utils.h)
Introduce Jsonizable CRTP base for JSON serialization/deserialization (jsonizable.h)
Tests
date_time_utils_test.cpp — Timestamp parsing, formatting, timezone handling
string_utils_test.cpp — String split, trim, conversion, and format operations
rapidjson_util_test.cpp — JSON document creation, read, and write
path_util_test.cpp — Path join, normalize, and UUID-based temp path generation
options_utils_test.cpp — Configuration option key-value parsing
jsonizable_test.cpp — JSON serialization and deserialization round-trip
API and Format
No API or format changes.
Documentation
No documentation changes
Generative AI tooling