Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
JDBC sources: consolidate date/time values mapping #20346
JDBC sources: consolidate date/time values mapping #20346
Changes from 35 commits
56660f1
f59cbb6
042a8cc
49a9051
907c25a
65a3633
a417a73
bdad242
218d507
a2a4f05
a9097bc
193f23c
39e5c9f
c5aadc4
23cab57
29bb71e
2025aa0
50a6657
1899f67
2d96bca
1adf00d
89149e7
73f28fa
0f81f9f
7c7d776
7f6748f
57e6604
427ee73
f15453e
048ec93
98caefd
1bf10b3
f1bbb61
d8c1d6d
982b40c
9441548
69300fc
211c6d1
cc78ece
0bb5454
241868f
57f5b4c
df82df8
7bcf4b7
bb35b6e
7a010f0
888fc45
defa844
de8865c
ba38011
f71f223
9bab7f4
92443bd
e67ad61
24263d8
1979b42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
does this handle time with timezone values correctly? E.g.
01:23:45.67Z
(asking because it's using a LocalTime object)(or am I misunderstanding how this method is supposed to be used?)
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.
Nope, it's related to
TIME
datatype only and not toTIMETZ
I'm wondering that we don't have common mapping in JdbcSourceOperations for TIMETZ and TIMESTAMPTZ datatypes to use it as a cursor field.
https://github.com/airbytehq/airbyte/blob/master/airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/JdbcSourceOperations.java#L66-L90
Currently it's implemented only for Postgres Source
I believe that we should have a separate task to use TIMETZ and TIMESTAMPTZ as a cursor for all JDBC sources to avoid breaking changes
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.
got it. Does that mean that even after this PR is merged,
timetz
values (for postgres + cockroachdb) will still beread
as full iso8601 timestamps?edit: or is this the special handling you added in
putCockroachSpecialDataType
, and postgres was already behaving correctly?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.
So this is fixing the
setTime
in the base class? If so, can you do the same and delete the redundant methods from inherited classes?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.
yes, that's right
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.
removed redundant methods from inherited classes for Postgres and MySQL