-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
🐛 📝 don't missparse +/-infinity in postgres-source #31856
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
e650134
to
b7737ab
Compare
@@ -634,23 +635,6 @@ protected void addTimeWithTimeZoneTest() { | |||
} | |||
} | |||
|
|||
protected void addTimestampWithInfinityValuesTest() { |
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 the test because 1) it was doing the wrong thing and 2) we added a lot more tests in the original test block
@@ -196,25 +196,6 @@ protected void addTimeWithTimeZoneTest() { | |||
} | |||
} | |||
|
|||
@Override | |||
protected void addTimestampWithInfinityValuesTest() { |
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 this because 1) it's testing the wrong thing and 2) we have a lot more infinity tests in the abstract class above
1e2de43
to
95d3cb7
Compare
553e00d
to
4733fac
Compare
"enum": ["+infinity", "-infinity"] | ||
} | ||
], | ||
"description": "An RFC 3339\u00a75.6 full-time, extended with +/-infinity" | ||
}, | ||
"TimeWithoutTimezone": { | ||
"type": "string", |
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.
oops
@@ -9,7 +9,7 @@ plugins { | |||
airbyteJavaConnector { | |||
cdkVersionRequired = '0.4.1' |
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.
push new CDK
Coverage report for source-postgres
|
features = ['db-sources'] | ||
useLocalCdk = false | ||
useLocalCdk = true |
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.
Reminder to unset this before merging.
/publish-java-cdk
|
/publish-java-cdk
|
0d1b3e6
to
d7b8e89
Compare
/publish-java-cdk
|
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 think we should comment that this isn't currently used (if that hasn't already been said)
8e61a39
to
58e6f4c
Compare
…postgres-source add tests. Add support for arrays of timestamps refactor more fixes Automated Commit - Formatting Changes fix add infinityHandling for CDC fix infinity for CDC Automated Commit - Formatting Changes simplify comparisons update CDK version revert change revert change change +/-infinity to (-)Infinity Update build.gradle Update version.properties revert last changes push CDK to 0.4.7
58e6f4c
to
3460729
Compare
/publish-java-cdk
|
fixes #19952
Postgres has the ability to store +infinity and -infinity in its
date
,timestamp
andtimestamp with timezone
datatypes. Unfortunately, we don't handle them, and PG-JDBC does some unnatural stuff to still return ajava.sql.*
java type: It returns a special value of a Timestamp/Date/OffsetDateTime, which is not their max value either. Debezium will do the same thing.So in this change, I look for those special values and return
+infinity
and-infinity
instead of a non-sensical date.There's some changes to the CDK because the postgres debezum converter is in there.
I added some tests to the
AbstractPostgresSourceDatatypeTest
, but I'm not 100% clear what the destination's behavior will be WRT the new values