-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[CALCITE-6252] BigQuery FORMAT_DATE uses the wrong calendar for Julian dates #3680
Conversation
// The Calendar and SimpleDateFormatter do not seem to give correct results | ||
// for the day of the week prior to the Julian to Gregorian date change. | ||
// So we resort to using a LocalDate representation. | ||
LocalDate ld = |
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.
Can use java.sql.Date directly?
((java.sql.Date) date).toLocalDate()
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 didn't know this trick, I will try it.
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.
it doesn't work, the subclass relationship goes the other way. sql.date extends util.date.
The calendar-based conversion seems to do the right thing and it's not too complicated.
final DateFormat eeeFormat = new SimpleDateFormat(DY.javaFmt, Locale.ROOT); | ||
final DateFormat mmmFormat = new SimpleDateFormat(MON.javaFmt, Locale.ROOT); | ||
final DateFormat mmmmFormat = new SimpleDateFormat(MONTH.javaFmt, Locale.ROOT); | ||
final DateFormat eeeFormat = new SimpleDateFormat(DY.javaFmt, Locale.US); |
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.
Why use Locale.US?
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.
SQL requires the output to be in English. Using Locale.ROOT gives the wrong result for the eeeeFormat at least (Mon instead of Monday).
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.
It looks good to me
@@ -281,9 +301,9 @@ static Work get() { | |||
|
|||
/** Uses Locale.US instead of Locale.ROOT to fix formatting in Java 11 */ |
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.
Hello, Should we change or add comments here?
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.
There are only Locale.ROOT comments here
b337021
to
9eb8508
Compare
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.
LGTM
…n dates Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
b202d5a
to
c3eeec9
Compare
Quality Gate passedIssues Measures |
This was much trickier to fix than I expected.
I think that it's because of some bugs in the Java Calendar/SimpleDateFormatter implementation.
Using the LocalDate class provides correct results.
I have also changed the return type of FORMAT_DATE from VARCHAR(2000) to VARCHAR, because it can really return results longer than 2000 characters, if the format string is long.