-
Notifications
You must be signed in to change notification settings - Fork 56
DBZ-7920 Add support for 'infinity' values for PostgreSQL #85
Conversation
Hi @mfvitale , one question. I see you marked do manual e2e test, what about these values from PG going to other database platforms? Should we introduce an E2E test that only applies to the PG source but can be run against the other sink targets for completeness? |
This is a good question. I just tried PostgreSQL to PostgreSQL. I'll add e2e for the others. |
So I suspect we likely will want to add some methods on the It looks like from what I quickly found:
|
Thanks for it. This open another question: should we manage also the opposite? These values should be converted to |
This is my final founding about the other databases support
|
@Naros can you give another look? |
That's a good question and I guess it would depend on the complexity. I haven't looked at the recent changes, but I could imagine that if the calendar limits for another vendor go beyond the limits of PostgreSQL, then it would make logical sense to convert those to +/- INFINITY since we would be incapable of writing the actual date value into PostgreSQL without an error. |
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.
LGTM, just a few inline comments & my comment above about translating to PG +/- INFINITY values.
@@ -23,34 +18,28 @@ | |||
* | |||
* @author Chris Cranford | |||
*/ | |||
public class ZonedTimestampType extends AbstractTimestampType { | |||
public class ZonedTimestampType extends io.debezium.connector.jdbc.type.debezium.ZonedTimestampType { |
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.
I must admit, I'm not a fan of extending a class with the same name; should we think about prefixing the derived class with a database prefix of sorts?
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.
Or we can prefix the base one with "Debezium" as done for "Connect" one. WDYT? I am open for both.
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.
Or we can prefix the base one with "Debezium" as done for "Connect" one.
Ya lets go with that approach, its more consistent with what we have already with prefixes, @mfvitale
src/test/java/io/debezium/connector/jdbc/e2e/AbstractJdbcSinkPipelineIT.java
Outdated
Show resolved
Hide resolved
src/test/java/io/debezium/connector/jdbc/e2e/AbstractJdbcSinkPipelineIT.java
Outdated
Show resolved
Hide resolved
The only issue seems to be only for the Oracle -infinity value (-4712-01-01T00:00:00+00:00). In that case maybe it is better to convert all infinity values from the source to the correct destination value. So I think it is better to manage this in a separate issue. WDYT? |
Makes sense to me. 👍 |
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.
Reran the tests for Oracle and they all checked out 👍, so LGTM.
|
closes: https://issues.redhat.com/browse/DBZ-7920
Missing task: