diff --git a/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp b/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp index e2b376a96abb..f7bc6d494c5d 100644 --- a/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp +++ b/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp @@ -517,10 +517,15 @@ TEST_F(DateTimeFunctionsTest, weekDate) { } TEST_F(DateTimeFunctionsTest, week) { - const auto weekTimestamp = [&](const char* time) { - auto timestampInSeconds = util::fromTimeString(time) / 1'000'000; + const auto weekTimestamp = [&](std::string_view time) { + auto ts = util::fromTimestampString( + time.data(), time.size(), util::TimestampParseMode::kIso8601) + .thenOrThrow(folly::identity, [&](const Status& status) { + VELOX_USER_FAIL("{}", status.message()); + }); auto timestamp = - std::make_optional(Timestamp(timestampInSeconds * 100'000'000, 0)); + std::make_optional(Timestamp(ts.getSeconds() * 100'000'000, 0)); + auto week = evaluateOnce("week(c0)", timestamp).value(); auto weekOfYear = evaluateOnce("week_of_year(c0)", timestamp).value(); @@ -529,19 +534,24 @@ TEST_F(DateTimeFunctionsTest, week) { return week; }; - EXPECT_EQ(1, weekTimestamp("00:00:00")); - EXPECT_EQ(10, weekTimestamp("11:59:59")); - EXPECT_EQ(51, weekTimestamp("06:01:01")); - EXPECT_EQ(24, weekTimestamp("06:59:59")); - EXPECT_EQ(27, weekTimestamp("12:00:01")); - EXPECT_EQ(7, weekTimestamp("12:59:59")); + EXPECT_EQ(1, weekTimestamp("T00:00:00")); + EXPECT_EQ(10, weekTimestamp("T11:59:59")); + EXPECT_EQ(51, weekTimestamp("T06:01:01")); + EXPECT_EQ(24, weekTimestamp("T06:59:59")); + EXPECT_EQ(27, weekTimestamp("T12:00:01")); + EXPECT_EQ(7, weekTimestamp("T12:59:59")); } TEST_F(DateTimeFunctionsTest, weekTimestampWithTimezone) { - const auto weekTimestampTimezone = [&](const char* time, + const auto weekTimestampTimezone = [&](std::string_view time, const char* timezone) { - auto timestampInSeconds = util::fromTimeString(time) / 1'000'000; - auto timestamp = timestampInSeconds * 100'000'000; + auto ts = util::fromTimestampString( + time.data(), time.size(), util::TimestampParseMode::kIso8601) + .thenOrThrow(folly::identity, [&](const Status& status) { + VELOX_USER_FAIL("{}", status.message()); + }); + + auto timestamp = ts.getSeconds() * 100'000'000; auto week = evaluateWithTimestampWithTimezone( "week(c0)", timestamp, timezone) .value(); @@ -553,14 +563,14 @@ TEST_F(DateTimeFunctionsTest, weekTimestampWithTimezone) { return week; }; - EXPECT_EQ(1, weekTimestampTimezone("00:00:00", "-12:00")); - EXPECT_EQ(1, weekTimestampTimezone("00:00:00", "+12:00")); - EXPECT_EQ(47, weekTimestampTimezone("11:59:59", "-12:00")); - EXPECT_EQ(47, weekTimestampTimezone("11:59:59", "+12:00")); - EXPECT_EQ(33, weekTimestampTimezone("06:01:01", "-12:00")); - EXPECT_EQ(34, weekTimestampTimezone("06:01:01", "+12:00")); - EXPECT_EQ(47, weekTimestampTimezone("12:00:01", "-12:00")); - EXPECT_EQ(47, weekTimestampTimezone("12:00:01", "+12:00")); + EXPECT_EQ(1, weekTimestampTimezone("T00:00:00", "-12:00")); + EXPECT_EQ(1, weekTimestampTimezone("T00:00:00", "+12:00")); + EXPECT_EQ(47, weekTimestampTimezone("T11:59:59", "-12:00")); + EXPECT_EQ(47, weekTimestampTimezone("T11:59:59", "+12:00")); + EXPECT_EQ(33, weekTimestampTimezone("T06:01:01", "-12:00")); + EXPECT_EQ(34, weekTimestampTimezone("T06:01:01", "+12:00")); + EXPECT_EQ(47, weekTimestampTimezone("T12:00:01", "-12:00")); + EXPECT_EQ(47, weekTimestampTimezone("T12:00:01", "+12:00")); } TEST_F(DateTimeFunctionsTest, quarter) { diff --git a/velox/type/TimestampConversion.cpp b/velox/type/TimestampConversion.cpp index 78ccca58ef25..46b2a6960979 100644 --- a/velox/type/TimestampConversion.cpp +++ b/velox/type/TimestampConversion.cpp @@ -738,36 +738,6 @@ fromTime(int32_t hour, int32_t minute, int32_t second, int32_t microseconds) { return result; } -int64_t fromTimeString(const char* str, size_t len) { - int64_t microsSinceMidnight; - size_t pos; - - if (!tryParseTimeString( - str, - len, - pos, - microsSinceMidnight, - TimestampParseMode::kPrestoCast)) { - VELOX_USER_FAIL( - "Unable to parse time value: \"{}\", " - "expected format is (HH:MM:SS[.MS])", - std::string_view(str, len)); - } - - // Check remaining string for non-space characters. - skipSpaces(str, len, pos); - - // Check position. If end was not reached, non-space chars remaining. - VELOX_USER_CHECK_EQ( - pos, - len, - "Unable to parse time value: \"{}\", " - "expected format is (HH:MM:SS[.MS])", - std::string_view(str, len)); - - return microsSinceMidnight; -} - Timestamp fromDatetime(int64_t daysSinceEpoch, int64_t microsSinceMidnight) { int64_t secondsSinceEpoch = static_cast(daysSinceEpoch) * kSecsPerDay; @@ -793,7 +763,7 @@ Status parserError(const char* str, size_t len) { Expected fromTimestampString(const char* str, size_t len, TimestampParseMode parseMode) { - size_t pos; + size_t pos = 0; Timestamp resultTimestamp; if (!tryParseTimestampString(str, len, pos, resultTimestamp, parseMode)) { diff --git a/velox/type/TimestampConversion.h b/velox/type/TimestampConversion.h index 45ced829267d..e5f6b5c4f715 100644 --- a/velox/type/TimestampConversion.h +++ b/velox/type/TimestampConversion.h @@ -132,18 +132,6 @@ int32_t extractISODayOfTheWeek(int32_t daysSinceEpoch); int64_t fromTime(int32_t hour, int32_t minute, int32_t second, int32_t microseconds); -// TODO These are used only in tests. Move them to test-only location. - -/// Parses the input string and returns the number of cumulative microseconds, -/// following the "HH:MM[:SS[.MS]]" format (ISO 8601). -// -/// Throws VeloxUserError if the format or time is invalid. -int64_t fromTimeString(const char* buf, size_t len); - -inline int64_t fromTimeString(const StringView& str) { - return fromTimeString(str.data(), str.size()); -} - // Timestamp conversion enum class TimestampParseMode { diff --git a/velox/type/tests/TimestampConversionTest.cpp b/velox/type/tests/TimestampConversionTest.cpp index 710b227c7925..2ea96844790e 100644 --- a/velox/type/tests/TimestampConversionTest.cpp +++ b/velox/type/tests/TimestampConversionTest.cpp @@ -188,49 +188,6 @@ TEST(DateTimeUtilTest, fromDateStringInvalid) { testCastFromDateStringInvalid("1970-01-01T01:00:47.000", ParseMode::kIso8601); } -TEST(DateTimeUtilTest, fromTimeString) { - EXPECT_EQ(0, fromTimeString("00:00:00")); - EXPECT_EQ(0, fromTimeString("00:00:00.00")); - EXPECT_EQ(1, fromTimeString("00:00:00.000001")); - EXPECT_EQ(10, fromTimeString("00:00:00.00001")); - EXPECT_EQ(100, fromTimeString("00:00:00.0001")); - EXPECT_EQ(1000, fromTimeString("00:00:00.001")); - EXPECT_EQ(10000, fromTimeString("00:00:00.01")); - EXPECT_EQ(100000, fromTimeString("00:00:00.1")); - EXPECT_EQ(1'000'000, fromTimeString("00:00:01")); - EXPECT_EQ(60'000'000, fromTimeString("00:01:00")); - EXPECT_EQ(3'600'000'000, fromTimeString("01:00:00")); - - // 1 day minus 1 second. - EXPECT_EQ(86'399'000'000, fromTimeString("23:59:59")); - - // Single digit. - EXPECT_EQ(0, fromTimeString("0:0:0.0")); - EXPECT_EQ(3'661'000'000, fromTimeString("1:1:1")); - - // Leading and trailing spaces. - EXPECT_EQ(0, fromTimeString(" \t \n 00:00:00.00 \t")); -} - -TEST(DateTimeUtilTest, fromTimeStrInvalid) { - const std::string errorMsg = "Unable to parse time value: "; - VELOX_ASSERT_THROW(fromTimeString(""), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00"), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00:"), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00:00:"), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00:00:00."), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00:00-00"), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00/00:00"), errorMsg); - - // Invalid hour, minutes and seconds. - VELOX_ASSERT_THROW(fromTimeString("24:00:00"), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00:61:00"), errorMsg); - VELOX_ASSERT_THROW(fromTimeString("00:00:61"), errorMsg); - - // Trailing characters. - VELOX_ASSERT_THROW(fromTimeString("00:00:00 12"), errorMsg); -} - // bash command to verify: // $ date -d "2000-01-01 12:21:56Z" +%s // ('Z' at the end means UTC).