-
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
MySQL/MsSQL Source: stop sync on null cursor value #23908
Conversation
# Conflicts: # airbyte-integrations/connectors/source-mysql/src/main/java/io/airbyte/integrations/source/mysql/MySqlSource.java
Affected Connector ReportNOTE
|
Connector | Version | Changelog | Publish |
---|---|---|---|
source-mssql |
1.0.2 |
✅ | ✅ |
source-mssql-strict-encrypt |
1.0.2 |
🔵 (ignored) |
🔵 (ignored) |
source-mysql |
2.0.2 |
✅ | ✅ |
source-mysql-strict-encrypt |
2.0.2 |
🔵 (ignored) |
🔵 (ignored) |
- See "Actionable Items" below for how to resolve warnings and errors.
✅ Destinations (0)
Connector | Version | Changelog | Publish |
---|
- See "Actionable Items" below for how to resolve warnings and errors.
✅ Other Modules (0)
Actionable Items
(click to expand)
Category | Status | Actionable Item |
---|---|---|
Version | ❌ mismatch |
The version of the connector is different from its normal variant. Please bump the version of the connector. |
⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
|
Changelog | ⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
❌ changelog missing |
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog. | |
Publish | ⚠ not in seed |
The connector is not in the seed file (e.g. source_definitions.yaml ), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug. |
❌ diff seed version |
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version. |
/test connector=connectors/source-mssql
Build PassedTest summary info:
|
""" | ||
SELECT t1.allowNulls & t2.hasNulls AS %s | ||
FROM (SELECT CAST(COLUMNPROPERTY(OBJECT_ID('%s.%s'), '%s', 'AllowsNull') AS BIT) AS allowNulls) AS t1 | ||
CROSS APPLY (SELECT CAST(IIF(EXISTS(SELECT TOP 1 1 FROM "%s"."%s" WHERE "%s" IS NULL), 1, 0) AS BIT) AS hasNulls) AS t2 |
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.
If t1 is 0
, would it still need to run the SELECT … WHERE … IS NULL
?
I'm not very familiar with sql server's CROSS APPLY
.
If possible we should avoid the potentially expensive SELECT for NULLs if column is not nullable.
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.
It looks like it will only do the t2 query if t1 is 1, because of the &
.
But just to verify
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.
The CROSS APPLY operator returns only those rows from the left table expression (in its final output) if it matches with the right table expression. In other words, the right table expression returns rows for the left table expression match only.
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, but do you know if it actually runs the right one (SELECT … IS NULL) if t1 bit is 0?
Would it still look for a match?
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.
@rodireich modified query to execute (SELECT … IS NULL) only in case of column is nullable
.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java
Outdated
Show resolved
Hide resolved
@@ -106,6 +114,43 @@ void testDiscoverWithPk() throws Exception { | |||
assertEquals(CATALOG, actual); | |||
} | |||
|
|||
@Test | |||
public void testTableWithNullCursorValueShouldThrowException() throws Exception { |
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.
One comment I got on the postgres test was to check also with a View
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.
updated with a view test
.../connectors/source-mysql/src/main/java/io/airbyte/integrations/source/mysql/MySqlSource.java
Outdated
Show resolved
Hide resolved
public static final String NULL_CURSOR_VALUE_WITH_SCHEMA_QUERY = | ||
""" | ||
SELECT | ||
(SELECT CAST(IIF(EXISTS(SELECT TOP 1 1 FROM FROM "%s"."%s" WHERE "%s" IS NULL), 1, 0) AS BIT) AS hasNulls |
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.
IIF
nice! 😃
Just wanted to check - has comms been sent to users who might be broken by this? @rodireich I remember you did this in stages where you (i) Logged the syncs and customers whose syncs would fail and (ii) Gave them a warning and then (iii) finally released the change. Has the same been done here? |
/test connector=connectors/source-mysql
Build PassedTest summary info:
|
…' into vmaltsev/23632-mysql-null-cursor
/publish connector=connectors/source-mssql-strict-encrypt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mssql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mysql-strict-encrypt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mysql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mysql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/test connector=connectors/source-mssql-strict-encrypt
Build PassedTest summary info:
|
…' into vmaltsev/23632-mysql-null-cursor
/test connector=connectors/source-mssql-strict-encrypt
Build PassedTest summary info:
|
# Conflicts: # airbyte-config/init/src/main/resources/seed/source_definitions.yaml # airbyte-config/init/src/main/resources/seed/source_specs.yaml # airbyte-integrations/connectors/source-mssql-strict-encrypt/Dockerfile # airbyte-integrations/connectors/source-mssql/Dockerfile # airbyte-integrations/connectors/source-mysql-strict-encrypt/Dockerfile # airbyte-integrations/connectors/source-mysql/Dockerfile # connectors.md # docs/integrations/sources/mssql.md # docs/integrations/sources/mysql.md
/publish connector=connectors/source-mssql-strict-encrypt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mysql-strict-encrypt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mssql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mysql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/test connector=connectors/source-mssql-strict-encrypt
Build PassedTest summary info:
|
/publish connector=connectors/source-mssql-strict-encrypt run-tests=false
if you have connectors that successfully published but failed definition generation, follow step 4 here |
…' into vmaltsev/23632-mysql-null-cursor
* MySQL Source: stop sync on null cursor value * updated MsSQL test * Fixed testTableWithNullCursorValueShouldThrowException test for Mssql * fixed MsSQL and MySQL tests * modified MsSQL query * fixed typo * fixed typo * simplified Mysql query logic * simplified Mssql query logic * add quotes to Mssql query * add mysql view test * MySQL and MsSQL source throw a warning if a cursor column contains null value * disabled tests; log warn if null cursor values found * removed logging in AbstractDbSource * bump version * bump version * auto-bump connector version * updated changelog * Automated Change * updated definitions * Automated Change --------- Co-authored-by: ievgeniit <etsybaev@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: VitaliiMaltsev <VitaliiMaltsev@users.noreply.github.com>
* MySQL Source: stop sync on null cursor value * updated MsSQL test * Fixed testTableWithNullCursorValueShouldThrowException test for Mssql * fixed MsSQL and MySQL tests * modified MsSQL query * fixed typo * fixed typo * simplified Mysql query logic * simplified Mssql query logic * add quotes to Mssql query * add mysql view test * MySQL and MsSQL source throw a warning if a cursor column contains null value * disabled tests; log warn if null cursor values found * removed logging in AbstractDbSource * bump version * bump version * auto-bump connector version * updated changelog * Automated Change * updated definitions * Automated Change --------- Co-authored-by: ievgeniit <etsybaev@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: VitaliiMaltsev <VitaliiMaltsev@users.noreply.github.com>
* MySQL Source: stop sync on null cursor value * updated MsSQL test * Fixed testTableWithNullCursorValueShouldThrowException test for Mssql * fixed MsSQL and MySQL tests * modified MsSQL query * fixed typo * fixed typo * simplified Mysql query logic * simplified Mssql query logic * add quotes to Mssql query * add mysql view test * MySQL and MsSQL source throw a warning if a cursor column contains null value * disabled tests; log warn if null cursor values found * removed logging in AbstractDbSource * bump version * bump version * auto-bump connector version * updated changelog * Automated Change * updated definitions * Automated Change --------- Co-authored-by: ievgeniit <etsybaev@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: VitaliiMaltsev <VitaliiMaltsev@users.noreply.github.com>
What
Issue 23632
🚨 User Impact 🚨
To notify users about the breaking changes temporal PR with logging should be merged first
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/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