Skip to content
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.

Sign up for GitHub

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

postgres-source: handle 24:00:00 value for time column #17782

Merged
merged 11 commits into from
Oct 11, 2022
Merged

postgres-source: handle 24:00:00 value for time column #17782

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c9fdbe3
postgres-source: handle 24:00:00 value for time column
subodh1810 Oct 10, 2022
a8d4918
Merge branch 'master' into handle-24-hour-time-postgres
subodh1810 Oct 10, 2022
9a3d3c7
address review comment
subodh1810 Oct 10, 2022
4426b0b
fix test + use LocalTime.MAX
subodh1810 Oct 10, 2022
eab2303
bump version
subodh1810 Oct 10, 2022
8fd1485
Merge branch 'master' into handle-24-hour-time-postgres
subodh1810 Oct 11, 2022
46d3d44
Merge branch 'master' into handle-24-hour-time-postgres
subodh1810 Oct 11, 2022
b302266
update alloy db as well
subodh1810 Oct 11, 2022
db24e7c
auto-bump connector version [ci skip]
octavia-squidington-iii Oct 11, 2022
7b64eef
auto-bump connector version [ci skip]
octavia-squidington-iii Oct 11, 2022
74222b2
Merge branch 'handle-24-hour-time-postgres' of https://github.com/air…
octavia-squidington-iii Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DateTimeConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ public static String convertToTime(final Object time) {
return localTime.format(TIME_FORMATTER);
} else if (time instanceof java.time.Duration) {
long value = ((Duration) time).toNanos();
if (value >= 0 && value <= TimeUnit.DAYS.toNanos(1)) {
if (value >= 0 && value < TimeUnit.DAYS.toNanos(1)) {
return LocalTime.ofNanoOfDay(value).format(TIME_FORMATTER);
} else {
final long updatedValue = 0 > value ? Math.abs(value) : TimeUnit.DAYS.toNanos(1);
final long updatedValue = 0 > value ? Math.abs(value) : 86399999999999L;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be more clear, assuming I understood correctly

Suggested change
final long updatedValue = 0 > value ? Math.abs(value) : 86399999999999L;
final long updatedValue = 0 > value ? Math.abs(value) : TimeUnit.DAYS.toNanos(1) - 1;

or maybe just this? I.e. convert negative values to positive, and then cap all values to less than 23:59 - this is slightly different from the current behavior, where -864000...0000 would be converted to 864000...0000 rather than being reduced to 86399999999999)

Suggested change
final long updatedValue = 0 > value ? Math.abs(value) : 86399999999999L;
final long updatedValue = Math.min(Math.abs(value), TimeUnit.DAYS.toNanos(1) - 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

LOGGER.debug("Time values must use number of milliseconds greater than 0 and less than 86400000000000 but its {}, converting to {} ", value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this comment say nanoseconds instead?

updatedValue);
return LocalTime.ofNanoOfDay(updatedValue).format(TIME_FORMATTER);
Expand All @@ -158,7 +158,13 @@ public static String convertToTime(final Object time) {
LOGGER.info("Unknown class for Time data type" + time.getClass());
loggedUnknownTimeClass = true;
}
return LocalTime.parse(time.toString()).format(TIME_FORMATTER);

final String valueAsString = time.toString();
if (valueAsString.startsWith("24")) {
LOGGER.debug("Time value {} is above range, converting to 23:59:59", valueAsString);
return LocalTime.parse("23:59:59.999999").format(TIME_FORMATTER);
Copy link
Contributor

@rodireich rodireich Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LocalTime has .MAX

}
return LocalTime.parse(valueAsString).format(TIME_FORMATTER);
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...integrations/io/airbyte/integration_tests/sources/AbstractPostgresSourceDatatypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,9 +404,9 @@ protected void initTests() {
.fullSourceDataType(fullSourceType)
.airbyteType(JsonSchemaType.STRING_TIME_WITHOUT_TIMEZONE)
// time column will ignore time zone
.addInsertValues("'13:00:01'", "'13:00:02+8'", "'13:00:03-8'", "'13:00:04Z'", "'13:00:05.01234Z+8'", "'13:00:00Z-8'")
.addInsertValues("'13:00:01'", "'13:00:02+8'", "'13:00:03-8'", "'13:00:04Z'", "'13:00:05.01234Z+8'", "'13:00:00Z-8'", "'24:00:00'")
.addExpectedValues("13:00:01.000000", "13:00:02.000000", "13:00:03.000000", "13:00:04.000000", "13:00:05.012340",
"13:00:00.000000")
"13:00:00.000000", "23:59:59.999999")
.build());
}

Expand Down