-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add helper function to return the number of days since epoch for a week-of-month date #10604
Changes from 21 commits
6104fc6
1d4667b
e4b2c78
b4504f8
e9be740
a4ff46c
555e1d9
83301ab
e718074
9ce7750
216a000
e850388
cb45539
a98fedd
6135991
b5f3be9
3165392
d3f2657
0e9412c
7a52b45
7d72017
3e2fb0c
0fcb111
0d29272
9267d21
935bab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ | |
#include <folly/Expected.h> | ||
#include "velox/common/base/CheckedArithmetic.h" | ||
#include "velox/common/base/Exceptions.h" | ||
#include "velox/type/HugeInt.h" | ||
#include "velox/type/tz/TimeZoneMap.h" | ||
|
||
namespace facebook::velox::util { | ||
|
@@ -144,6 +145,53 @@ bool isValidWeekDate(int32_t weekYear, int32_t weekOfYear, int32_t dayOfWeek) { | |
return true; | ||
} | ||
|
||
bool isValidWeekOfMonthDate( | ||
int32_t year, | ||
int32_t month, | ||
int32_t weekOfMonth, | ||
int32_t dayOfWeek) { | ||
rui-mo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (year < 1 || year > kMaxYear) { | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return false; | ||
} | ||
if (month < 1 || month > 12) { | ||
return false; | ||
} | ||
|
||
int64_t daysSinceEpochOfFirstDayOfMonth; | ||
const Status status = | ||
daysSinceEpochFromDate(year, month, 1, daysSinceEpochOfFirstDayOfMonth); | ||
if (!status.ok()) { | ||
return false; | ||
} | ||
|
||
// Calculates the actual number of week of month and validates if it is in the | ||
// valid range. | ||
const int32_t firstDayOfWeek = | ||
extractISODayOfTheWeek(daysSinceEpochOfFirstDayOfMonth); | ||
const int32_t firstWeekLength = 7 - firstDayOfWeek + 1; | ||
const int32_t monthLength = | ||
isLeapYear(year) ? kLeapDays[month] : kNormalDays[month]; | ||
const int32_t actualWeeks = 1 + ceil((monthLength - firstWeekLength) / 7.0); | ||
if (weekOfMonth < 1 || weekOfMonth > actualWeeks) { | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return false; | ||
} | ||
|
||
// Validate day of week. | ||
rui-mo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// If dayOfWeek is before the first day of week, it is considered invalid. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to match the documentation, which suggests the valid range is fixed [1, 7]. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. The documentation is not clear. Thanks. |
||
if (weekOfMonth == 1 && dayOfWeek < firstDayOfWeek) { | ||
return false; | ||
} | ||
const int32_t lastWeekLength = (monthLength - firstWeekLength) % 7; | ||
// If dayOfWeek is after the last day of the last week of the month, it is | ||
// considered invalid. | ||
if (weekOfMonth == actualWeeks && lastWeekLength != 0 && | ||
dayOfWeek > lastWeekLength) { | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
inline bool validDate(int64_t daysSinceEpoch) { | ||
return daysSinceEpoch >= std::numeric_limits<int32_t>::min() && | ||
daysSinceEpoch <= std::numeric_limits<int32_t>::max(); | ||
|
@@ -593,6 +641,62 @@ Status daysSinceEpochFromWeekDate( | |
return Status::OK(); | ||
} | ||
|
||
Expected<int64_t> daysSinceEpochFromWeekOfMonthDate( | ||
int32_t year, | ||
int32_t month, | ||
int32_t weekOfMonth, | ||
int32_t dayOfWeek, | ||
bool lenient) { | ||
if (!lenient && | ||
!isValidWeekOfMonthDate(year, month, weekOfMonth, dayOfWeek)) { | ||
if (threadSkipErrorDetails()) { | ||
return folly::makeUnexpected(Status::UserError()); | ||
} else { | ||
return folly::makeUnexpected(Status::UserError( | ||
"Date out of range: {}-{}-{}-{}", | ||
year, | ||
month, | ||
weekOfMonth, | ||
dayOfWeek)); | ||
} | ||
} | ||
|
||
// Adjusts the year and month to ensure month is within the range 1-12, | ||
// accounting for overflow or underflow. | ||
int32_t additionYears = 0; | ||
if (month < 1) { | ||
additionYears = month / 12 - 1; | ||
month = 12 - abs(month) % 12; | ||
} else if (month > 12) { | ||
additionYears = (month - 1) / 12; | ||
month = (month - 1) % 12 + 1; | ||
} | ||
year += additionYears; | ||
|
||
int64_t daysSinceEpochOfFirstDayOfMonth; | ||
const Status status = | ||
daysSinceEpochFromDate(year, month, 1, daysSinceEpochOfFirstDayOfMonth); | ||
rui-mo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!status.ok()) { | ||
if (threadSkipErrorDetails()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is not needed, just return folly::makeUnexpected(status); We assume that the function that returned 'status' already checked this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I will add the check to daysSinceEpochFromDate. |
||
return folly::makeUnexpected(Status::UserError()); | ||
} else { | ||
return folly::makeUnexpected(status); | ||
} | ||
} | ||
const int32_t firstDayOfWeek = | ||
extractISODayOfTheWeek(daysSinceEpochOfFirstDayOfMonth); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I notice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the cast is to avoid overflow in addition and negating. If we change it as int64_t, overflow needs to considered as well by casting to a larger type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your explanation. I see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there is any limitation for year? If the days cannot exceed INT32_MAX, it would be safe to call it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The max year is 292278994 so the days can be (292278994-1970)*365=106,681,113,760 that is bigger than 2,147,483,647(INT32_MAX). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. @mbasmanova What are your thoughts? Thanks. |
||
int32_t days; | ||
if (dayOfWeek < 1) { | ||
days = 7 - abs(dayOfWeek - 1) % 7; | ||
} else if (dayOfWeek > 7) { | ||
days = (dayOfWeek - 1) % 7; | ||
} else { | ||
days = dayOfWeek % 7; | ||
} | ||
return daysSinceEpochOfFirstDayOfMonth - (firstDayOfWeek - 1) + | ||
7 * (weekOfMonth - 1) + days - 1; | ||
} | ||
|
||
Status | ||
daysSinceEpochFromDayOfYear(int32_t year, int32_t dayOfYear, int64_t& out) { | ||
if (!isValidDayOfYear(year, dayOfYear)) { | ||
|
@@ -643,7 +747,7 @@ Expected<int32_t> fromDateString(const char* str, size_t len, ParseMode mode) { | |
return daysSinceEpoch; | ||
} | ||
|
||
int32_t extractISODayOfTheWeek(int32_t daysSinceEpoch) { | ||
int32_t extractISODayOfTheWeek(int64_t daysSinceEpoch) { | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// date of 0 is 1970-01-01, which was a Thursday (4) | ||
// -7 = 4 | ||
// -6 = 5 | ||
|
@@ -662,10 +766,10 @@ int32_t extractISODayOfTheWeek(int32_t daysSinceEpoch) { | |
// 7 = 4 | ||
if (daysSinceEpoch < 0) { | ||
// negative date: start off at 4 and cycle downwards | ||
return (7 - ((-int64_t(daysSinceEpoch) + 3) % 7)); | ||
return (7 - ((-int128_t(daysSinceEpoch) + 3) % 7)); | ||
} else { | ||
// positive date: start off at 4 and cycle upwards | ||
return ((int64_t(daysSinceEpoch) + 3) % 7) + 1; | ||
return ((int128_t(daysSinceEpoch) + 3) % 7) + 1; | ||
rui-mo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,38 @@ Status daysSinceEpochFromWeekDate( | |
int32_t dayOfWeek, | ||
int64_t& out); | ||
|
||
/// Computes the signed number of days since the Unix epoch (1970-01-01). To | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// align with Spark's SimpleDateFormat behavior, this function offers two | ||
/// modes: lenient and non-lenient. For non-lenient mode, it returns an error | ||
/// status if the date is invalid. For lenient mode, it accepts a wider range of | ||
/// arguments. | ||
/// @param year Year. For non-lenient mode, it should be in the range [1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
typo? The range doesn't seem to allow negative values or zero. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a typo. The min value of year in non-lenient mode is 1. Here is Java implementation:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NEUpanning Got it. In this case "e.g: 1996, -2000." needs updating since -2000 is not allowed. It might be helpful to re-iterate that dates before Jan 1, 1970 are not supported in non-lenient mode. I find it hard to say / write non-lenient. Would it make sense to flip the boolean to 'strict' mode? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mbasmanova I assume using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow. I was suggesting to consider replacing 'lenient' boolean with 'strict'. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spark uses the term 'lenient', and this function aligns with Spark. See link. Therefore, I think 'lenient' would be more appropriate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NEUpanning I assume this suggestion is about replacing 'non-lenient' with 'strict' which makes it more straightforward to understand. The 'lenient' will stay unchanged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is my concern. If this is not a problem or rarely happens, I'd like to replace it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NEUpanning Since it has been clarified in the comment that there are only two modes, so I think it is fine. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your suggestion. I've replaced it. |
||
/// 292278994]. e.g: 1996, -2000. For lenient mode, values outside this range | ||
/// could result in overflow. | ||
rui-mo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// @param month Month of year. For non-lenient mode, it should be in the range | ||
/// [1, 12]. For example, 1 is January, 7 is July. For lenient mode, values | ||
/// greater than 12 wrap around to the start of the year, and values less than 1 | ||
/// count backward from December. For example, 13 corresponds to January of the | ||
/// following year and -1 corresponds to November of the previous year. | ||
/// @param weekOfMonth Week of the month. For non-lenient mode, it should be in | ||
/// the range [1, 6]. For example, 1 is 1st week, 3 is 3rd week. For lenient | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// mode, we consider days of the previous or next months as part of the | ||
/// specified weekOfMonth. For example, if weekOfMonth is 5 but the current | ||
/// month only has 4 weeks (such as February), the first week of March will be | ||
/// considered as the 5th week of February. | ||
/// @param dayOfWeek Day number of week. For non-lenient mode, it should be in | ||
/// the range [1, 7]. For example, 1 is Monday, 7 is Sunday. For lenient mode, | ||
/// we consider days of the previous or next months as part of the specified | ||
/// dayOfWeek.For example, if weekOfMonth is 1 and dayOfWeek is 1 but the | ||
/// month's first day is Saturday, the Monday of the last week of the previous | ||
/// month will be used. | ||
Expected<int64_t> daysSinceEpochFromWeekOfMonthDate( | ||
int32_t year, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation imposes limits on the 'year' value. Would you document these? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Done. |
||
int32_t month, | ||
int32_t weekOfMonth, | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int32_t dayOfWeek, | ||
bool lenient); | ||
|
||
/// Computes the (signed) number of days since unix epoch (1970-01-01). | ||
/// Returns UserError status if the date is invalid. | ||
Status | ||
|
@@ -123,7 +155,7 @@ inline Expected<int32_t> fromDateString(const StringView& str, ParseMode mode) { | |
} | ||
|
||
// Extracts the day of the week from the number of days since epoch | ||
int32_t extractISODayOfTheWeek(int32_t daysSinceEpoch); | ||
int32_t extractISODayOfTheWeek(int64_t daysSinceEpoch); | ||
|
||
/// Time conversions. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,6 +104,100 @@ TEST(DateTimeUtilTest, fromDateInvalid) { | |
1970, 6, 31, "Date out of range: 1970-6-31")); | ||
} | ||
|
||
TEST(DateTimeUtilTest, fromWeekOfMonthDate) { | ||
auto daysSinceEpochLenient = | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[](int32_t year, int32_t month, int32_t weekOfMonth, int32_t dayOfWeek) { | ||
auto result = util::daysSinceEpochFromWeekOfMonthDate( | ||
year, month, weekOfMonth, dayOfWeek, true); | ||
EXPECT_TRUE(!result.hasError()); | ||
return result.value(); | ||
}; | ||
|
||
EXPECT_EQ(4, daysSinceEpochLenient(1970, 1, 2, 1)); | ||
rui-mo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
EXPECT_EQ(361, daysSinceEpochLenient(1971, 1, 1, 1)); | ||
EXPECT_EQ(396, daysSinceEpochLenient(1971, 2, 1, 1)); | ||
|
||
EXPECT_EQ(10952, daysSinceEpochLenient(2000, 1, 1, 1)); | ||
EXPECT_EQ(19905, daysSinceEpochLenient(2024, 7, 1, 1)); | ||
|
||
// Before unix epoch. | ||
EXPECT_EQ(-3, daysSinceEpochLenient(1970, 1, 1, 1)); | ||
EXPECT_EQ(-2, daysSinceEpochLenient(1970, 1, 1, 2)); | ||
EXPECT_EQ(-31, daysSinceEpochLenient(1969, 12, 1, 1)); | ||
EXPECT_EQ(-367, daysSinceEpochLenient(1969, 1, 1, 1)); | ||
EXPECT_EQ(-724, daysSinceEpochLenient(1968, 1, 2, 1)); | ||
EXPECT_EQ(-719533, daysSinceEpochLenient(0, 1, 1, 1)); | ||
|
||
// Negative year - BC. | ||
EXPECT_EQ(-719561, daysSinceEpochLenient(-1, 12, 1, 1)); | ||
EXPECT_EQ(-719897, daysSinceEpochLenient(-1, 1, 1, 1)); | ||
|
||
// Day in the previous month. | ||
EXPECT_EQ(19783, daysSinceEpochLenient(2024, 2, 5, 5)); | ||
// Day in the next month. | ||
EXPECT_EQ(19751, daysSinceEpochLenient(2024, 2, 1, 1)); | ||
|
||
// Out of range day of week. | ||
EXPECT_EQ(338, daysSinceEpochLenient(1970, 12, 1, 0)); | ||
EXPECT_EQ(337, daysSinceEpochLenient(1970, 12, 1, -1)); | ||
EXPECT_EQ(337, daysSinceEpochLenient(1970, 12, 1, -8)); | ||
|
||
EXPECT_EQ(332, daysSinceEpochLenient(1970, 12, 1, 8)); | ||
EXPECT_EQ(333, daysSinceEpochLenient(1970, 12, 1, 9)); | ||
EXPECT_EQ(336, daysSinceEpochLenient(1970, 12, 1, 19)); | ||
|
||
// Out of range month. | ||
EXPECT_EQ(-3, daysSinceEpochLenient(1970, 1, 1, 1)); | ||
EXPECT_EQ(207, daysSinceEpochLenient(1970, 8, 1, 1)); | ||
EXPECT_EQ(361, daysSinceEpochLenient(1970, 13, 1, 1)); | ||
|
||
EXPECT_EQ(-31, daysSinceEpochLenient(1970, 0, 1, 1)); | ||
EXPECT_EQ(-66, daysSinceEpochLenient(1970, -1, 1, 1)); | ||
EXPECT_EQ(-430, daysSinceEpochLenient(1970, -13, 1, 1)); | ||
|
||
// Out of range year. | ||
auto result = | ||
util::daysSinceEpochFromWeekOfMonthDate(292278995, 1, 1, 1, true); | ||
EXPECT_EQ(result.error().message(), "Date out of range: 292278995-1-1"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we do not expect errors in lenient mode. This behavior doesn't seem to match the doc. Would you double check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can see this conversation #10604 (comment). Maybe I should change the documentation. |
||
} | ||
|
||
TEST(DateTimeUtilTest, extractISODayOfTheWeek) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason this tests is added in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function extractISODayOfTheWeek is changed. See this discussion:#10604 (comment) |
||
EXPECT_EQ( | ||
4, util::extractISODayOfTheWeek(std::numeric_limits<int64_t>::max())); | ||
EXPECT_EQ( | ||
3, util::extractISODayOfTheWeek(std::numeric_limits<int64_t>::min())); | ||
EXPECT_EQ(1, util::extractISODayOfTheWeek(-10)); | ||
EXPECT_EQ(7, util::extractISODayOfTheWeek(10)); | ||
} | ||
|
||
TEST(DateTimeUtilTest, fromWeekOfMonthDateInvalid) { | ||
auto daysSinceEpochNonLenient = [](int32_t year, | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int32_t month, | ||
int32_t weekOfMonth, | ||
int32_t dayOfWeek, | ||
const std::string& error) { | ||
auto result = util::daysSinceEpochFromWeekOfMonthDate( | ||
year, month, weekOfMonth, dayOfWeek, false); | ||
EXPECT_TRUE(result.error().isUserError()); | ||
EXPECT_EQ(result.error().message(), error); | ||
}; | ||
|
||
EXPECT_NO_THROW( | ||
daysSinceEpochNonLenient(-1, 1, 1, 1, "Date out of range: -1-1-1-1")); | ||
NEUpanning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
EXPECT_NO_THROW(daysSinceEpochNonLenient( | ||
292278995, 1, 1, 1, "Date out of range: 292278995-1-1-1")); | ||
EXPECT_NO_THROW( | ||
daysSinceEpochNonLenient(2024, 0, 1, 1, "Date out of range: 2024-0-1-1")); | ||
EXPECT_NO_THROW(daysSinceEpochNonLenient( | ||
2024, 13, 1, 1, "Date out of range: 2024-13-1-1")); | ||
EXPECT_NO_THROW( | ||
daysSinceEpochNonLenient(2024, 1, 6, 1, "Date out of range: 2024-1-6-1")); | ||
EXPECT_NO_THROW( | ||
daysSinceEpochNonLenient(2024, 2, 1, 1, "Date out of range: 2024-2-1-1")); | ||
EXPECT_NO_THROW( | ||
daysSinceEpochNonLenient(2024, 2, 5, 5, "Date out of range: 2024-2-5-5")); | ||
} | ||
|
||
TEST(DateTimeUtilTest, fromDateString) { | ||
for (ParseMode mode : {ParseMode::kPrestoCast, ParseMode::kSparkCast}) { | ||
EXPECT_EQ(0, parseDate("1970-01-01", mode)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the discussion at https://github.com/facebookincubator/velox/pull/10511/files#r1686566980, true result does not always indicate a valid date.
With current implementation, for some invalid date, we return user error status while for the others we return a int64_t result in the
daysSinceEpochFromWeekOfMonthDate
function, which seems to be confusing.I notice the method
isValidDate
provides an accurate evaluation for valid date. Does it make sense to use that?https://github.com/facebookincubator/velox/blob/main/velox/type/TimestampConversion.cpp#L518-L529
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. We should remove the validation for week of month. According to SimpleDateFormat's doc the range of
day
is [1,7] soisValidDate
isn't suitable for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. See e718074
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify the purpose of
isValidWeekOfMonthDate
? Is it to follow certain rule defined in SimpleDateFormat to check if a date is valid, and what are the rules? Perhaps add some comments for this function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to do validation follows SimpleDateFormat. However, SimpleDateFormat does not provide clear rules or relevant documentation. For example, document states
WEEK_OF_MONTH
field range is from 0 to 6, , but in my tests, a value of 99 is considered valid.(https://github.com/facebookincubator/velox/pull/10511/files#r1686566980)Another example is the valid range for the
year
field, which is not documented.I tried to determine the valid range through testing, but it is too costly and not reliable enough. I wonder how to properly validate dates in Velox. Could you give me some advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rui-mo I don't believe this functionality is needed for Presto functions. Hence, we are free to define semantics in any "sensible" way. Assuming we need these semantics to match Spark, we can use Spark semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer. Understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbasmanova @rui-mo According to this discussion, Spark date validation rules in document don't match with the test results. Which one should I follow? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NEUpanning I assume we need to follow the actual behavior of Spark. How do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rui-mo I agree with you. I will change the part of date validation for following the actual behavior of Spark.