-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[Source-Mssql] Format Datetime and Datetime2 datatypes to 6-digit microsecond precision #32573
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,
|
0d4cd18
to
7fe6c3d
Compare
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.
just one small comment
try { | ||
node.put(columnName, getObject(resultSet, index, LocalDateTime.class).format(microsecondsFormatter)); | ||
} catch (final Exception e) { | ||
// for backward compatibility |
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.
what does that mean? Should we at least log something here ?
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.
After thinking about it, I'm going to remove this section altogether. This was done inside of AbstractJdbcCompatibleSourceOperations#putTimestamp
for generic JDBC sources because we didn't know if we were able to parse the result to the appropriate Datetime/Date/Timestamp
Java type. But since we know how a MSSQL datetime result looks like, this isn't necessary.
9b0dc29
to
73449e8
Compare
2007a67
to
f61375e
Compare
private final Set<String> BINARY = Set.of("VARBINARY", "BINARY"); | ||
private final Set<String> DATETIME_TYPES = Set.of("DATETIME", "DATETIME2", "SMALLDATETIME"); |
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 see also date
, datetimeoffset
, time
.
Any other one also relevant?
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.
Those 3 are the only ones that gets translated into timestamp
. Datetimeoffset
is timestamp with timezone, and the remaining date
and time
have their own translation.
private static final String DATETIMEOFFSET = "DATETIMEOFFSET"; | ||
private static final String TIME_TYPE = "TIME"; | ||
private static final String SMALLMONEY_TYPE = "SMALLMONEY"; | ||
private static final String GEOMETRY = "GEOMETRY"; | ||
private static final String GEOGRAPHY = "GEOGRAPHY"; | ||
private static final String DEBEZIUM_DATETIMEOFFSET_FORMAT = "yyyy-MM-dd HH:mm:ss XXX"; | ||
|
||
private static final String DATETIME_FORMAT_MICROSECONDS = "yyyy-MM-dd'T'HH:mm:ss[.][SSSSSS]"; |
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.
Why 6?
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.
We're hard coding it to 6 to fit with most of the popular destinations for now. This work is slated to be reverted once Destination/platform establish a precision standard on their end.
@@ -6,7 +6,7 @@ plugins { | |||
airbyteJavaConnector { | |||
cdkVersionRequired = '0.5.0' | |||
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 change before merging
adc9e2b
to
ddcddc7
Compare
…ow how MSSQL datetime results look like
eaad3d8
to
e1eebd9
Compare
/publish-java-cdk
|
d56b5fc
to
83106c6
Compare
Description: SQLServer's
datetime2
data type has a max timestamp precision of 7, but for some destinations, specifically BigQuery, sending a record with this precision - e.g:2023-11-08T01:20:11.3733338
- will be rejected by BigQuery with a thrown error, and this leads to a dropped record on the other end as the entry will show up as nullThis also updated
datetime
to have the same precision for consistency.Temporary solution is to enforce a 6-digit precision after reading the record from the source before data emission.
Note: This is to temporarily unblock a potential customer. This will be reverted when Destination/Platform start to handle this issue on their end.