Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
tanclary merged PR #3761: URL: https://github.com/apache/calcite/pull/3761 -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
mihaibudiu commented on PR #3761: URL: https://github.com/apache/calcite/pull/3761#issuecomment-2064785139 This PR seems to implement a few flags which don't even work in BigQuery, but other than that seems fine. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on PR #3761: URL: https://github.com/apache/calcite/pull/3761#issuecomment-2064728779 @mihaibudiu @tanclary addressed your comments, would appreciate another review on this, thanks! -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1571090898
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -32,8 +32,15 @@ EXPR$0
2017-05-01 01:23:45
!ok
+# Input that contains shuffled date without time
+select cast('12-2010-05' as timestamp format
+'DD--MM');
+EXPR$0
+2010-05-12 00:00:00
+!ok
+
!if (false) {
-### disabled until Bug.CALCITE_6269_FIXED ###
+### unsupported format model/elements, test disabled ###
Review Comment:
Updated the comments and disable condition to point to the calcite issue,
thanks for suggesting this change!
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1571090026
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -306,12 +446,22 @@ static Work get() {
* https://issues.apache.org/jira/browse/CALCITE-6252. This may be
* specific to Java 11. */
final DateFormat Format = new SimpleDateFormat(MONTH.javaFmt,
Locale.US);
-final DateFormat sFormat = new SimpleDateFormat(FF1.javaFmt, Locale.ROOT);
-final DateFormat ssFormat = new SimpleDateFormat(FF2.javaFmt, Locale.ROOT);
final DateFormat sssFormat = new SimpleDateFormat(FF3.javaFmt,
Locale.ROOT);
-final DateFormat Format = new SimpleDateFormat(FF4.javaFmt,
Locale.ROOT);
-final DateFormat sFormat = new SimpleDateFormat(FF5.javaFmt,
Locale.ROOT);
-final DateFormat ssFormat = new SimpleDateFormat(FF6.javaFmt,
Locale.ROOT);
final DateFormat yyFormat = new SimpleDateFormat(YY.javaFmt, Locale.ROOT);
+final DateFormat Format = new SimpleDateFormat(.javaFmt,
Locale.ROOT);
+
+/** Util to return the full or abbreviated weekday name from date and
expected TextStyle. */
+public String getDayFromDate(Date date, TextStyle style) {
Review Comment:
Good catch, can be kept private updated it!
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1571079145
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -217,6 +291,34 @@ public enum FormatElementEnum implements FormatElement {
sb.append(String.format(Locale.ROOT, "%d", (calendar.get(Calendar.MONTH)
/ 3) + 1));
}
},
+ AMPM("", "The time as Meridian Indicator in uppercase") {
+@Override public void format(StringBuilder sb, Date date) {
+ final Calendar calendar = Work.get().calendar;
+ calendar.setTime(date);
+ sb.append(calendar.get(Calendar.AM_PM) == Calendar.AM ? "AM" : "PM");
Review Comment:
Apologies, not an enum but the class stores these calendar elements as
member properties of type int, so the conversion to string is still necessary.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1571074953
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -217,6 +291,34 @@ public enum FormatElementEnum implements FormatElement {
sb.append(String.format(Locale.ROOT, "%d", (calendar.get(Calendar.MONTH)
/ 3) + 1));
}
},
+ AMPM("", "The time as Meridian Indicator in uppercase") {
+@Override public void format(StringBuilder sb, Date date) {
+ final Calendar calendar = Work.get().calendar;
+ calendar.setTime(date);
+ sb.append(calendar.get(Calendar.AM_PM) == Calendar.AM ? "AM" : "PM");
Review Comment:
Checked, `Calendar` in an INT enum, so the returned value needs to be
compared and output as a string explicitly, as the enum only contains its int
value.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761: URL: https://github.com/apache/calcite/pull/3761#discussion_r1571070432 ## core/src/test/resources/sql/cast-with-format.iq: ## @@ -979,7 +939,7 @@ EXPR$0 select cast(date'2019-01-07' as varchar FORMAT 'WW'); EXPR$0 -01 +02 Review 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. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
tanclary commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1567660915
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -217,6 +291,34 @@ public enum FormatElementEnum implements FormatElement {
sb.append(String.format(Locale.ROOT, "%d", (calendar.get(Calendar.MONTH)
/ 3) + 1));
}
},
+ AMPM("", "The time as Meridian Indicator in uppercase") {
+@Override public void format(StringBuilder sb, Date date) {
+ final Calendar calendar = Work.get().calendar;
+ calendar.setTime(date);
+ sb.append(calendar.get(Calendar.AM_PM) == Calendar.AM ? "AM" : "PM");
Review Comment:
can you just call `.toString()` on calendar.get(am_pm)` or no
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -306,12 +446,22 @@ static Work get() {
* https://issues.apache.org/jira/browse/CALCITE-6252. This may be
* specific to Java 11. */
final DateFormat Format = new SimpleDateFormat(MONTH.javaFmt,
Locale.US);
-final DateFormat sFormat = new SimpleDateFormat(FF1.javaFmt, Locale.ROOT);
-final DateFormat ssFormat = new SimpleDateFormat(FF2.javaFmt, Locale.ROOT);
final DateFormat sssFormat = new SimpleDateFormat(FF3.javaFmt,
Locale.ROOT);
-final DateFormat Format = new SimpleDateFormat(FF4.javaFmt,
Locale.ROOT);
-final DateFormat sFormat = new SimpleDateFormat(FF5.javaFmt,
Locale.ROOT);
-final DateFormat ssFormat = new SimpleDateFormat(FF6.javaFmt,
Locale.ROOT);
final DateFormat yyFormat = new SimpleDateFormat(YY.javaFmt, Locale.ROOT);
+final DateFormat Format = new SimpleDateFormat(.javaFmt,
Locale.ROOT);
+
+/** Util to return the full or abbreviated weekday name from date and
expected TextStyle. */
+public String getDayFromDate(Date date, TextStyle style) {
Review Comment:
should this be private
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
mihaibudiu commented on code in PR #3761: URL: https://github.com/apache/calcite/pull/3761#discussion_r1566470361 ## core/src/test/resources/sql/cast-with-format.iq: ## @@ -979,7 +939,7 @@ EXPR$0 select cast(date'2019-01-07' as varchar FORMAT 'WW'); EXPR$0 -01 +02 Review Comment: Since this is BigQuery, they better give the same results. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761: URL: https://github.com/apache/calcite/pull/3761#discussion_r1566469451 ## core/src/test/resources/sql/cast-with-format.iq: ## @@ -979,7 +939,7 @@ EXPR$0 select cast(date'2019-01-07' as varchar FORMAT 'WW'); EXPR$0 -01 +02 Review 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. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1566466811
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -253,12 +373,27 @@ public enum FormatElementEnum implements FormatElement {
sb.append(String.format(Locale.ROOT, "%02d",
calendar.get(Calendar.WEEK_OF_YEAR)));
}
},
+ Y("y", "Last digit of year") {
+@Override public void format(StringBuilder sb, Date date) {
+ final Work work = Work.get();
+ String formattedYear = work.yyFormat.format(date);
+ sb.append(formattedYear.substring(formattedYear.length() - 1));
+}
+ },
YY("yy", "Last 2 digits of year") {
@Override public void format(StringBuilder sb, Date date) {
final Work work = Work.get();
sb.append(work.yyFormat.format(date));
}
},
+ YYY("yyy", "Last 3 digits of year") {
+@Override public void format(StringBuilder sb, Date date) {
+ 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));
Review Comment:
You are right, the earlier method was reusing what was being done in ``
and cropping that down to three digits. But the `` 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 `` 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.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
mihaibudiu commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1566455854
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -32,8 +32,15 @@ EXPR$0
2017-05-01 01:23:45
!ok
+# Input that contains shuffled date without time
+select cast('12-2010-05' as timestamp format
+'DD--MM');
+EXPR$0
+2010-05-12 00:00:00
+!ok
+
!if (false) {
-### disabled until Bug.CALCITE_6269_FIXED ###
+### unsupported format model/elements, test disabled ###
Review 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)`
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1566452703
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -32,8 +32,15 @@ EXPR$0
2017-05-01 01:23:45
!ok
+# Input that contains shuffled date without time
+select cast('12-2010-05' as timestamp format
+'DD--MM');
+EXPR$0
+2010-05-12 00:00:00
+!ok
+
!if (false) {
-### disabled until Bug.CALCITE_6269_FIXED ###
+### unsupported format model/elements, test disabled ###
Review 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?
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1566447732
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -361,7 +351,7 @@ EXPR$0
select cast('2017-05-31' as
timestamp FORMAT '-MM-DD');
EXPR$0
-2017-05-31 00:00:00
+0017-05-31 00:00:00
Review Comment:
Thanks for catching that, I accidentally edited that line instead of one
below.. have refactored some of this code.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
Anthrino commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1566422922
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -1155,7 +1108,7 @@ EXPR$0
!ok
!if (false) {
-### disabled until Bug.CALCITE_6269_FIXED ###
+### unsupported format model/elements, test disabled ###
Review Comment:
Agreed I think I missed that one, good catch!
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6269] Fix missing/broken BigQuery date-time format elements [calcite]
mihaibudiu commented on code in PR #3761:
URL: https://github.com/apache/calcite/pull/3761#discussion_r1566225540
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -1155,7 +1108,7 @@ EXPR$0
!ok
!if (false) {
-### disabled until Bug.CALCITE_6269_FIXED ###
+### unsupported format model/elements, test disabled ###
Review Comment:
this seems the same as the test in the change above on line 1091 above which
is enabled
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -361,7 +351,7 @@ EXPR$0
select cast('2017-05-31' as
timestamp FORMAT '-MM-DD');
EXPR$0
-2017-05-31 00:00:00
+0017-05-31 00:00:00
Review Comment:
why is this correct?
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -32,8 +32,15 @@ EXPR$0
2017-05-01 01:23:45
!ok
+# Input that contains shuffled date without time
+select cast('12-2010-05' as timestamp format
+'DD--MM');
+EXPR$0
+2010-05-12 00:00:00
+!ok
+
!if (false) {
-### disabled until Bug.CALCITE_6269_FIXED ###
+### unsupported format model/elements, test disabled ###
Review 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.
##
core/src/test/resources/sql/cast-with-format.iq:
##
@@ -979,7 +939,7 @@ EXPR$0
select cast(date'2019-01-07' as varchar
FORMAT 'WW');
EXPR$0
-01
+02
Review Comment:
what is happening here?
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -253,12 +373,27 @@ public enum FormatElementEnum implements FormatElement {
sb.append(String.format(Locale.ROOT, "%02d",
calendar.get(Calendar.WEEK_OF_YEAR)));
}
},
+ Y("y", "Last digit of year") {
+@Override public void format(StringBuilder sb, Date date) {
+ final Work work = Work.get();
+ String formattedYear = work.yyFormat.format(date);
+ sb.append(formattedYear.substring(formattedYear.length() - 1));
+}
+ },
YY("yy", "Last 2 digits of year") {
@Override public void format(StringBuilder sb, Date date) {
final Work work = Work.get();
sb.append(work.yyFormat.format(date));
}
},
+ YYY("yyy", "Last 3 digits of year") {
+@Override public void format(StringBuilder sb, Date date) {
+ 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));
Review Comment:
what happens for a year like 50?
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
