Move common implementation to base implementation #33369

gisripa · 2023-12-12T20:38:13Z

What

Promotes common implementation to base class and redshift specific overrides.
Anything jooq supports with dialect specific syntax is moved to JdbcSqlGenerator.
Breaking CDK changes.

Closes #31591

vercel · 2023-12-12T20:38:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Dec 13, 2023 10:00pm

github-actions · 2023-12-12T20:38:36Z

Before Merging a Connector Pull Request

Wow! 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:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

gisripa · 2023-12-12T20:42:19Z

...main/java/io/airbyte/cdk/integrations/destination/jdbc/typing_deduping/JdbcSqlGenerator.java

+
+  protected abstract Condition cdcDeletedAtNotNullCondition();
+
+  protected abstract Field<Integer> getRowNumber(final List<ColumnId> primaryKey, final Optional<ColumnId> cursorField);


jooq already handles dialect specific rowNumber() window step function but the additional support of DESC NULLS FIRST|LAST is missing (which redshift and snowflake does). so, made it abstract

edgao · 2023-12-12T20:53:37Z

is it possible to extract a JdbcSqlGeneratorIntegrationTest class? I feel like that's the last piece missing (and also huge leverage, iirc implementing the redshift version took nearly 2 days)

gisripa · 2023-12-12T21:51:08Z

is it possible to extract a JdbcSqlGeneratorIntegrationTest class? I feel like that's the last piece missing (and also huge leverage, iirc implementing the redshift version took nearly 2 days)

I think so, I'll take a look. expected fixtures will still be destination specific but some of the setup/teardown code can go to this class.

edgao

took a quick pass over this. Left a few comments but didn't read the sqlgenerators in great detail. I think this is really great though, love that everything in RedshiftSqlGenerator is actually redshift-specific now ❤️

will do another pass later but I think this is enough for me to write up some tickets for mysql :)

...main/java/io/airbyte/cdk/integrations/destination/jdbc/typing_deduping/JdbcSqlGenerator.java

edgao · 2023-12-12T22:51:43Z

...main/java/io/airbyte/cdk/integrations/destination/jdbc/typing_deduping/JdbcSqlGenerator.java

+    if (!force) {
+      return Strings.join(
+          List.of(
+              createSchemaSql(stream.id().finalNamespace()),


(nonblocking) this will need to be refactored after @jbfbell 's #33124, but should be a pretty easy change.

...main/java/io/airbyte/cdk/integrations/destination/jdbc/typing_deduping/JdbcSqlGenerator.java

gisripa · 2023-12-13T01:30:02Z

...o/airbyte/integrations/base/destination/typing_deduping/JdbcSqlGeneratorIntegrationTest.java

+import org.jooq.meta.TableDefinition;
+
+public abstract class JdbcSqlGeneratorIntegrationTest<T> extends BaseSqlGeneratorIntegrationTest<T> {
+


@edgao Moved reusable methods to base class. Thing to be noted is typing-deduping doesn't depend on CDK so i had to leave it generic type. I think these methods should be moved to a Util rather than a base test class in later refactors.

we could put this into db-destinations.testFixtures?

edgao

approving just so we can get this thing merged, but we probably should add the useExpensiveSaferCasting logic soonish

edgao · 2023-12-13T19:00:21Z

...main/java/io/airbyte/cdk/integrations/destination/jdbc/typing_deduping/JdbcSqlGenerator.java

+  private String insertAndDeleteTransaction(final StreamConfig streamConfig,
+                                            final String finalSuffix,
+                                            final Optional<Instant> minRawTimestamp,
+                                            final boolean useExpensiveSaferCasting) {


intellij says this param isn't being used?

gisripa · 2023-12-13T21:20:03Z

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/7201193180
❌ Publish Java CDK version=0.7.2 failed!

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

…dbc sqls Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

gisripa · 2023-12-13T21:37:30Z

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/7201359317
✅ Successfully published Java CDK version=0.7.3!

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

gisripa requested a review from a team as a code owner December 12, 2023 20:38

octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/destination/redshift labels Dec 12, 2023

gisripa commented Dec 12, 2023

View reviewed changes

gisripa mentioned this pull request Dec 12, 2023

Destination redshift v2: fix migration + add test #33367

Merged

edgao reviewed Dec 12, 2023

View reviewed changes

gisripa commented Dec 13, 2023

View reviewed changes

gisripa force-pushed the gireesh/jdbc_v2 branch from e914adb to aa02722 Compare December 13, 2023 02:34

vercel bot deployed to Preview December 13, 2023 18:08 View deployment

edgao approved these changes Dec 13, 2023

View reviewed changes

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Dec 13, 2023

vercel bot deployed to Preview December 13, 2023 21:03 View deployment

vercel bot deployed to Preview December 13, 2023 21:06 View deployment

gisripa added the checklist-action-run label Dec 13, 2023

gisripa added 8 commits December 13, 2023 13:31

Move common implementation to JdbcSqlGenerator.java

8c24aa7

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

Move common setup/teardown tasks to base which are common for other J…

af4d92f

…dbc sqls Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

Minor changes to cdc_deleted_at columnId visibility

7da11a7

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

Manually resurrecting merge conflict changes.

9e403c6

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

Format 🤷

065c25a

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

Connector version bump

f59f90d

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

formatting again 🤷

c950d3a

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

javadoc and cdk version change

f392129

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

edgao mentioned this pull request Dec 13, 2023

DV2: Only run T+D if we have zero records or the previous sync left behind un-T+D-ed records #33232

Merged

gisripa force-pushed the gireesh/jdbc_v2 branch from c4d5155 to f392129 Compare December 13, 2023 21:31

bump cdk version

4de29e8

:omg: formatting

ac751be

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

gisripa added checklist-action-run and removed checklist-action-run labels Dec 13, 2023

Merge branch 'master' into gireesh/jdbc_v2

4d2ee29

vercel bot deployed to Preview December 13, 2023 21:58 View deployment

use published cdk

acb3f8f

gisripa merged commit 4cec594 into master Dec 13, 2023
21 checks passed

gisripa deleted the gireesh/jdbc_v2 branch December 13, 2023 22:33

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

Move common implementation to base implementation (airbytehq#33369)

2c2e9a6

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

Move common implementation to base implementation (airbytehq#33369)

af60f8b

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

Move common implementation to base implementation (airbytehq#33369)

9e3d7ce

Signed-off-by: Gireesh Sreepathi <gisripa@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move common implementation to base implementation #33369

Move common implementation to base implementation #33369

gisripa commented Dec 12, 2023 •

edited

vercel bot commented Dec 12, 2023 •

edited

github-actions bot commented Dec 12, 2023 •

edited by gisripa

gisripa Dec 12, 2023

edgao commented Dec 12, 2023

gisripa commented Dec 12, 2023

edgao left a comment

edgao Dec 12, 2023

gisripa Dec 13, 2023

edgao Dec 13, 2023

edgao left a comment

edgao Dec 13, 2023

gisripa commented Dec 13, 2023 •

edited by github-actions bot

gisripa commented Dec 13, 2023 •

edited by github-actions bot


		protected abstract Condition cdcDeletedAtNotNullCondition();

		protected abstract Field<Integer> getRowNumber(final List<ColumnId> primaryKey, final Optional<ColumnId> cursorField);

		import org.jooq.meta.TableDefinition;

		public abstract class JdbcSqlGeneratorIntegrationTest<T> extends BaseSqlGeneratorIntegrationTest<T> {

Move common implementation to base implementation #33369

Move common implementation to base implementation #33369

Conversation

gisripa commented Dec 12, 2023 • edited

What

vercel bot commented Dec 12, 2023 • edited

github-actions bot commented Dec 12, 2023 • edited by gisripa

Before Merging a Connector Pull Request

gisripa Dec 12, 2023

Choose a reason for hiding this comment

edgao commented Dec 12, 2023

gisripa commented Dec 12, 2023

edgao left a comment

Choose a reason for hiding this comment

edgao Dec 12, 2023

Choose a reason for hiding this comment

gisripa Dec 13, 2023

Choose a reason for hiding this comment

edgao Dec 13, 2023

Choose a reason for hiding this comment

edgao left a comment

Choose a reason for hiding this comment

edgao Dec 13, 2023

Choose a reason for hiding this comment

gisripa commented Dec 13, 2023 • edited by github-actions bot

gisripa commented Dec 13, 2023 • edited by github-actions bot

gisripa commented Dec 12, 2023 •

edited

vercel bot commented Dec 12, 2023 •

edited

github-actions bot commented Dec 12, 2023 •

edited by gisripa

gisripa commented Dec 13, 2023 •

edited by github-actions bot

gisripa commented Dec 13, 2023 •

edited by github-actions bot