-
Notifications
You must be signed in to change notification settings - Fork 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
🐛 Source MSSQL: fix data type (smalldatetime, smallmoney) conversion from mssql source (#5609) #7386
🐛 Source MSSQL: fix data type (smalldatetime, smallmoney) conversion from mssql source (#5609) #7386
Conversation
/test connector=connectors/source-mssql
|
...c/main/resources/config/STANDARD_SOURCE_DEFINITION/b5ea17b1-f170-46dc-bc31-cc744ca984c1.json
Show resolved
Hide resolved
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
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.
Good PR.
I'm only worrying about DebeziumConverterUtils.java and possible conflicts.
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public final class DebeziumConverterUtils { |
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.
There will be a conflict with PR #7339 where DebeziumConverterUtils.java is also added!
This change should be consolidated in order to avoid merge conflicts and any effects on earlier merge code.
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.
Agree. I will apply changes from #7339 once it is merged to master
@@ -26,6 +26,10 @@ static Properties getDebeziumProperties() { | |||
// https://debezium.io/documentation/reference/1.4/connectors/sqlserver.html#sqlserver-property-provide-transaction-metadata | |||
props.setProperty("provide.transaction.metadata", "false"); | |||
|
|||
props.setProperty("converters", "smalldatetime, smallmoney"); | |||
props.setProperty("smalldatetime.type", "io.airbyte.integrations.debezium.internals.MSSQLConverter"); |
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.
you should not put multiply properties for the same converter.
there is no relation between converter name and actual types which are covered by it. Please leave only one.
https://debezium.io/documentation/reference/development/converters.html
P.S. I agree that our names confuse mostly and would be better to use converter name.type
instead.
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.
Fixed
/test connector=connectors/source-mssql
|
623f680
to
72172cb
Compare
72172cb
to
8df87e2
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.
LGTM. we can proceed with publish/merge without airbyte review
/test connector=connectors/source-mssql
|
/test connector=connectors/source-mssql-strict-encrypt
|
/publish connector=connectors/source-mssql
|
/publish connector=connectors/source-mssql-strict-encrypt
|
/publish connector=connectors/source-mssql-strict-encrypt
|
…from mssql source (airbytehq#5609) (airbytehq#7386) * Fix data type (smalldatetime, smallmoney) conversion from mssql source (airbytehq#5609) * Fixed code format * Bumb new version * Update documentation (mssql.md) * formating * fixed converter properties * aligned converter utils with airbytehq#7339 Co-authored-by: Andrii Leonets <30464745+DoNotPanicUA@users.noreply.github.com>
What
Fixed data type (
smalldatetime
,smallmoney
) conversion from mssql source (#5609)How
Created custom MSSQLConverter that implements CustomConverter
Recommended reading order
MSSQLConverter.java
DebeziumConverterUtils.java
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes