Calendar API changes so that methods accepting strings return strings #5029

chipkent · 2024-01-10T20:05:51Z

Suggested changes to the Calendar API so that methods that accept string inputs return strings.

…ring

jjbrosnan · 2024-01-11T14:17:52Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -1287,7 +1291,7 @@ public LocalDate[] businessDates(final LocalDate start, final LocalDate end) {
     * @throws InvalidDateException if the dates are not in the valid range
     * @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
     */
-    public LocalDate[] businessDates(final String start, final String end) {
+    public String[] businessDates(final String start, final String end) {


Every other businessDate overload returns a LocalDate[]. I don't think the one that takes string inputs needs to be special.

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

jjbrosnan · 2024-01-11T14:18:08Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -1357,14 +1361,16 @@ public LocalDate[] nonBusinessDates(final LocalDate start, final LocalDate end,
     * @throws InvalidDateException if the dates are not in the valid range
     * @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
     */
-    public LocalDate[] nonBusinessDates(final String start, final String end, final boolean startInclusive,
+    public String[] nonBusinessDates(final String start, final String end, final boolean startInclusive,


Same comment as the one on businessDates

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

jjbrosnan · 2024-01-11T14:19:07Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -1430,7 +1436,7 @@ public LocalDate[] nonBusinessDates(final LocalDate start, final LocalDate end)
     * @throws InvalidDateException if the dates are not in the valid range
     * @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
     */
-    public LocalDate[] nonBusinessDates(final String start, final String end) {
+    public String[] nonBusinessDates(final String start, final String end) {


Same comment as the one I made on businessDates

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

jjbrosnan · 2024-01-11T14:24:41Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -165,6 +165,7 @@ private YearData getYearData(final int year) {

    // region Getters

+    // TODO: should there be a string equivalent?


I don't think there needs to be one

I agree; people can convert these to strings on their own, or stick with java.time types (as we'd generally recommend).

I don't strongly oppose a firstValidDateStr(); I just wouldn't add it proactively. We can always add it later if people ask for it.

jjbrosnan · 2024-01-11T14:24:59Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -1214,14 +1216,16 @@ public LocalDate[] businessDates(final LocalDate start, final LocalDate end, fin
     * @throws InvalidDateException if the dates are not in the valid range
     * @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
     */
-    public LocalDate[] businessDates(final String start, final String end, final boolean startInclusive,
+    public String[] businessDates(final String start, final String end, final boolean startInclusive,


Same comment as on the other overload.

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

I think String in -> String out, LocalDate in -> LocalDate out is the most user-friendly

jjbrosnan · 2024-01-11T14:25:23Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -174,6 +175,7 @@ public LocalDate firstValidDate() {
        return firstValidDate;
    }

+    // TODO: should there be a string equivalent?


I don't think so.

jjbrosnan · 2024-01-11T14:26:06Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -2091,6 +2099,7 @@ public ZonedDateTime minusNonBusinessDays(final ZonedDateTime time, final int da
        return plusNonBusinessDays(time, -days);
    }

+    // TODO: should there be a string equivalent?


I don't think so.

jjbrosnan · 2024-01-11T14:26:16Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -2108,6 +2117,7 @@ public LocalDate futureBusinessDate(final int days) {
        return plusBusinessDays(calendarDate(), days);
    }

+    // TODO: should there be a string equivalent?


I don't think so.

jjbrosnan · 2024-01-11T14:26:30Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -2125,6 +2135,7 @@ public LocalDate pastBusinessDate(final int days) {
        return minusBusinessDays(calendarDate(), days);
    }

+    // TODO: should there be a string equivalent?


I don't think so.

jjbrosnan · 2024-01-11T14:27:28Z

engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java

@@ -89,6 +90,7 @@ public String toString() {

    // region Now

+    // TODO: should there be a string equivalent?


I don't think so

jjbrosnan · 2024-01-11T14:28:15Z

engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java

@@ -117,6 +119,7 @@ public DayOfWeek dayOfWeek(final LocalDate date) {
        return DateTimeUtils.dayOfWeek(date);
    }

+    // TODO: should this return a String?


All of the overloads for this method return a DayOfWeek. The one that takes string as input doesn't have to be special. It should conform to the behavior the overloads have.

jjbrosnan · 2024-01-11T14:29:29Z

engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java

@@ -392,6 +397,7 @@ public ZonedDateTime minusDays(final ZonedDateTime time, final int days) {
        return plusDays(time, -days);
    }

+    // TODO: should there be a string equivalent?


I don't think so.

jjbrosnan · 2024-01-11T14:29:36Z

engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java

@@ -403,6 +409,7 @@ public LocalDate futureDate(final int days) {
        return plusDays(calendarDate(), days);
    }

+    // TODO: should there be a string equivalent?


I don't think so.

jjbrosnan

There were four methods in BusinessCalendar that I noticed behavior differed between overloads:

minusBusinessDays
minusNonBusinessDays
plusBusinessDays
plusNonBusinessDays

Those four have 4 overloads each - in all but 1 overload, the method returns the same type that's passed in. The only one where that's not the case is when a String is passed in. In that case, a LocalDate is returned. Those are the only 4 methods I think should change so that the behavior is consistent across all overloads.

After glancing at Calendar as well, there are two methods where I think this should change in the same way:

minusDays
plusDays

rbasralian

Looks good to me — String in -> String out, LocalDate in -> LocalDate out, and always LocalDate if there are no date arguments.

It does make me wonder if we should make parseLocalDate() accept nulls — it would be nice to be able to do parseLocalDate(someFunctionResultOrJoinedOnColumnThatMayBeNull) whenever you have a string date, without having to handle nulls manually. We have parseLocalDateQuiet() (and similar for other parse methods), but those also swallow format exceptions.

rbasralian · 2024-01-11T21:15:02Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -165,6 +165,7 @@ private YearData getYearData(final int year) {

    // region Getters

+    // TODO: should there be a string equivalent?


I agree; people can convert these to strings on their own, or stick with java.time types (as we'd generally recommend).

I don't strongly oppose a firstValidDateStr(); I just wouldn't add it proactively. We can always add it later if people ask for it.

rbasralian · 2024-01-11T21:16:23Z

engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

@@ -1214,14 +1216,16 @@ public LocalDate[] businessDates(final LocalDate start, final LocalDate end, fin
     * @throws InvalidDateException if the dates are not in the valid range
     * @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
     */
-    public LocalDate[] businessDates(final String start, final String end, final boolean startInclusive,
+    public String[] businessDates(final String start, final String end, final boolean startInclusive,


I think String in -> String out, LocalDate in -> LocalDate out is the most user-friendly

deephaven-internal · 2024-01-12T15:15:37Z

Labels indicate documentation is required. Issues for documentation have been opened:

Community: https://github.com/deephaven/deephaven.io/issues/3616

chipkent added 5 commits January 10, 2024 08:49

Fix broken calendar unit tests over midnight.

64076f9

Resolves deephaven#5024.

Spotless

a22292d

Addressed reviewer comments.

8ba59b4

Converted string input methods to return strings.

8cd97a4

TODOs that raise question.

76164a1

chipkent added query engine DocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Jan 10, 2024

chipkent added this to the 1. January 2024 (end of month) milestone Jan 10, 2024

chipkent requested review from rbasralian and jjbrosnan January 10, 2024 20:05

chipkent self-assigned this Jan 10, 2024

chipkent added 2 commits January 10, 2024 13:07

Merge remote-tracking branch 'origin/main' into calendar_string_to_st…

18ed1fd

…ring

Spotless

847acd6

jjbrosnan reviewed Jan 11, 2024

View reviewed changes

rbasralian reviewed Jan 11, 2024

View reviewed changes

Remove todos.

d7cee84

chipkent modified the milestones: 1. January 2024 (end of month), 0. December 2023 (end of month) Jan 12, 2024

jjbrosnan approved these changes Jan 12, 2024

View reviewed changes

chipkent merged commit d11f342 into deephaven:main Jan 12, 2024
19 checks passed

chipkent deleted the calendar_string_to_string branch January 12, 2024 15:15

github-actions bot locked and limited conversation to collaborators Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calendar API changes so that methods accepting strings return strings #5029

Calendar API changes so that methods accepting strings return strings #5029

chipkent commented Jan 10, 2024

jjbrosnan Jan 11, 2024

chipkent Jan 11, 2024

jjbrosnan Jan 11, 2024

chipkent Jan 11, 2024

jjbrosnan Jan 11, 2024

chipkent Jan 11, 2024

jjbrosnan Jan 11, 2024

rbasralian Jan 11, 2024

jjbrosnan Jan 11, 2024

chipkent Jan 11, 2024

rbasralian Jan 11, 2024

jjbrosnan Jan 11, 2024

jjbrosnan Jan 11, 2024 •

edited

Loading

jjbrosnan Jan 11, 2024 •

edited

Loading

jjbrosnan Jan 11, 2024 •

edited

Loading

jjbrosnan Jan 11, 2024

jjbrosnan Jan 11, 2024

jjbrosnan Jan 11, 2024

jjbrosnan Jan 11, 2024

jjbrosnan left a comment •

edited

Loading

rbasralian left a comment

rbasralian Jan 11, 2024

rbasralian Jan 11, 2024

deephaven-internal commented Jan 12, 2024

		@@ -165,6 +165,7 @@ private YearData getYearData(final int year) {

		// region Getters

		// TODO: should there be a string equivalent?

		@@ -89,6 +90,7 @@ public String toString() {

		// region Now

		// TODO: should there be a string equivalent?

Calendar API changes so that methods accepting strings return strings #5029

Calendar API changes so that methods accepting strings return strings #5029

Conversation

chipkent commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbrosnan Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

jjbrosnan Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

jjbrosnan Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbrosnan left a comment • edited Loading

Choose a reason for hiding this comment

rbasralian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deephaven-internal commented Jan 12, 2024

jjbrosnan Jan 11, 2024 •

edited

Loading

jjbrosnan Jan 11, 2024 •

edited

Loading

jjbrosnan Jan 11, 2024 •

edited

Loading

jjbrosnan left a comment •

edited

Loading