[CALCITE-6269] Fix missing/broken BigQuery date-time format elements #3761
Conversation
Anthrino
force-pushed
the
CALCITE_2980_cast_op_format_clause
branch
from
6e4ec7d
to
760f6d6
Compare
| final Calendar calendar = Work.get().calendar; | ||
| calendar.setTime(date); | ||
| String formattedYear = String.format(Locale.ROOT, "%d", calendar.get(Calendar.YEAR)); | ||
| sb.append(formattedYear.substring(formattedYear.length() - 3)); |
There was a problem hiding this comment.
what happens for a year like 50?
There was a problem hiding this comment.
You are right, the earlier method was reusing what was being done in YYYY and cropping that down to three digits. But the YYYY implementation was incorrect as it was reading an integer where the Calendar object stored the year, and that would return an int value and not a formatted year with 4 digits (showing the century number).
Corrected the YYYY and YYY functions to return a proper formatted value in terms of digits, and added a method pctY to return values consistent to what BQ returns.
| !if (false) { | ||
| ### disabled until Bug.CALCITE_6269_FIXED ### | ||
| ### unsupported format model/elements, test disabled ### |
There was a problem hiding this comment.
There is a way in Quidem tests to refer to a Bug, why don't we use that one?
The good thing about the previous version of this string is that you can grep for it, here it is not clear which bug tracks the problem.
There was a problem hiding this comment.
@mihaibudiu good point, I was also unsure of this comment and ideally wanted to link JIRA issues which would describe the exact causes. But there are quite a number of problems with the current implementation which is causing these many tests to fail and would take time to document them all, what do you recommend be done in this scenario?
There was a problem hiding this comment.
I think it's best to have an issue to keep track of all of them, even if they are different, and to have an easy way to find it in the code.
The syntax in .iq files is: !if (fixed.calcite1048)
There was a problem hiding this comment.
Updated the comments and disable condition to point to the calcite issue, thanks for suggesting this change!
| @@ -979,7 +939,7 @@ EXPR$0 | |||
| select cast(date'2019-01-07' as varchar | |||
| FORMAT 'WW'); | |||
| EXPR$0 | |||
| 01 | |||
| 02 | |||
There was a problem hiding this comment.
what is happening here?
There was a problem hiding this comment.
The WW operator returns the week number of the year (00-53), and that depends on what is being set as the start day of a week (Sun v/s Mon). The variances you see here are because we set the week start on Sunday, but setting to Monday also fails a bunch of other tests, which is strange as you would expect all of them to be consistent. So we have to use either of them and update the tests accordingly.
There was a problem hiding this comment.
Since this is BigQuery, they better give the same results.
There was a problem hiding this comment.
Agreed, a small number of the tests return the same answer as recorded here as they produced the same week number irrespective of start of week, the others had to be changed wrt to what Calcite/BQ is returning.
Another point to consider with these tests is BQ does not support use of elements like WW, DDD to parse a datetime object, its only available for formatting output strings. I left these tests as is because we do not have any restrictions in calcite's FormatModel for the use of elements in parsing or formatting scenarios.
Anthrino
force-pushed
the
CALCITE_2980_cast_op_format_clause
branch
from
760f6d6
to
585b1fc
Compare
There was a problem hiding this comment.
Overall think this looks good ! I agree with @mihaibudiu 's comments and left a couple questions of my own.
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java
Outdated
Show resolved
Hide resolved
Anthrino
force-pushed
the
CALCITE_2980_cast_op_format_clause
branch
from
585b1fc
to
4916779
Compare
|
@mihaibudiu @tanclary addressed your comments, would appreciate another review on this, thanks! |
There was a problem hiding this comment.
LGTM if @mihaibudiu doesn't have any comments I think this is good to go
|
This PR seems to implement a few flags which don't even work in BigQuery, but other than that seems fine. |
No description provided.