ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4251987457
##########
cpp/src/arrow/csv/converter.cc:
##########
@@ -464,18 +474,156 @@ struct MultipleParsersTimestampValueDecoder : public
ValueDecoder {
}
protected:
- using ParserVector = std::vector<const TimestampParser*>;
+ TimeUnit::type unit_;
+ bool expect_timezone_;
+ std::vector<const TimestampParser*> parsers_;
+};
+
+//
+// Value decoder for dates and times, with fallback to user-defined
+// timestamp parsers
+//
+
+// Splits the fractional seconds (up to 9 digits) off an ISO-8601 timestamp
+// "YYYY-MM-DD[ T]hh:mm:ss.s{1,9}" with an optional zone offset. Returns `s`
+// if there are no fractional seconds, a copy without them in `buffer`, or
+// nullptr if they are malformed. The split-off digits are returned in
+// `subseconds`.
+const char* SplitISO8601Subseconds(const char* s, uint32_t* size, char
(&buffer)[64],
+ std::string_view* subseconds) {
+ if (*size <= 20 || s[19] != '.') {
+ return s;
+ }
+ uint32_t fraction_length = 0;
+ while (20 + fraction_length < *size && s[20 + fraction_length] >= '0' &&
+ s[20 + fraction_length] <= '9') {
+ ++fraction_length;
+ }
+ const uint32_t rest_length = *size - 20 - fraction_length;
+ if (fraction_length == 0 || fraction_length > 9 || 19 + rest_length >
sizeof(buffer)) {
+ return nullptr;
+ }
+ std::memcpy(buffer, s, 19);
+ std::memcpy(buffer + 19, s + 20 + fraction_length, rest_length);
Review Comment:
A possibly simpler way to fix this: date and time columns reject zone
offsets anyway, so the only valid thing after the fractional digits is the end
of the value. If `SplitISO8601Subseconds` requires the fraction to run to the
end, it can just shorten `size` to 19 instead of copying the rest into a
buffer. That rejects a second fraction outright and also removes the 64-byte
stack buffer and both `memcpy` calls:
```cpp
// Splits the fractional seconds (1 to 9 digits) off the end of an ISO-8601
// timestamp "YYYY-MM-DD[ T]hh:mm:ss.s{1,9}" into `subseconds`, and shortens
// `size` to exclude them. Returns false if the fractional seconds are
// malformed or followed by anything (zone offsets are rejected for dates and
// times anyway).
bool SplitISO8601Subseconds(const char* s, uint32_t* size,
std::string_view* subseconds) {
if (*size <= 20 || s[19] != '.') {
return true;
}
const uint32_t fraction_length = *size - 20;
if (fraction_length > 9) {
return false;
}
for (uint32_t i = 20; i < *size; ++i) {
if (s[i] < '0' || s[i] > '9') {
return false;
}
}
*subseconds = std::string_view(s + 20, fraction_length);
*size = 19;
return true;
}
```
with the call site becoming:
```cpp
if (std::strcmp(parser->kind(), "iso8601") == 0 &&
!SplitISO8601Subseconds(s, &s_size, &subseconds)) {
continue;
}
```
The existing tests that reject zone offsets after a fraction (`.5Z`,
`.5+05`, `.5+0530`, `.5+05:30`) should still pass, since those values now fail
at the split instead of in the parser. The regression tests for `2020-03-15
14:30:00.5.5` and `2020-03-15 23:59:59.9.9` would still be good to add.
--
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]
