Support multi-row inserts #107

hariso · 2021-12-17T18:19:36Z

Hello, Aiven team!

This is in relation to #40 (and, originally #34). It takes the spike in #39 from @ahmeroxa as a basis.

Multi-row insert is implemented in a separate dialect and added to only Pg and SQLite. I wasn't comfortable adding multi-row support to GenericDatabaseDialect, since I wasn't able to actually confirm that all databases support it. I found that for example, PostgreSQL didn't support this until 8.2 But if we can, for example, assume that it's only the minority of DBs (and versions) which do not support it, then that could be a reason for putting multi-row inserts into GenericDatabaseDialect.

Best wishes,

Haris

Update: Since it looks like the most used DBs (MySQL, Pg, SQL Server) all support multi-row inserts, the default for the dialects is to support multi-row inserts.

hariso · 2022-01-17T11:43:41Z

Bump.: )

juha-aiven

Hey, thanks for the PR.

I had a quick initial look and left some comments.

I wasn't comfortable adding multi-row support to GenericDatabaseDialect, since I wasn't able to actually confirm that all databases support it.

What would happen if the multi-row support was added to all dialects and some DB didn't support it? The inserts would fail and things would crash?

Would it make sense to include the support "globally" and then it would be a "user error" if his DB didn't support it?

It seems that e.g. MySQL supports multi-row insert. I wouldn't like to take the route of not supporting it for a DB that has the feature, e.g. MySQL.

So, either we should double check which ones support it and add support for those or just add the support globally and it wouldn't just work if the underlying DB didn't support it.

juha-aiven · 2022-01-20T09:20:30Z

build.gradle

@@ -117,6 +118,7 @@ dependencies {

    implementation "com.google.guava:guava:27.1-jre"
    implementation "org.slf4j:slf4j-api:$slf4jVersion"
+    implementation "com.amazon.redshift:redshift-jdbc42:2.1.0.1"


Is this really needed in implementation-scope?

This might have even escaped from another PR I plan to open (a Redshift dialect). I think I can remove it completely.

juha-aiven · 2022-01-20T09:24:02Z

src/test/resources/log4j.properties

@@ -22,4 +23,4 @@ log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
 log4j.appender.stdout.layout.ConversionPattern=[%d] %p %m (%c:%L)%n

 log4j.logger.org.apache.kafka=ERROR
-log4j.logger.io.aiven.connect=ERROR
+log4j.logger.io.aiven.connect=DEBUG


Why change this? I think if you face errors while developing, you can change the log level. I'd revert the change in this file.

Makes sense. 👍

juha-aiven · 2022-01-20T09:26:02Z

src/main/java/io/aiven/connect/jdbc/dialect/DatabaseDialect.java

@@ -1,6 +1,7 @@
 /*
 * Copyright 2020 Aiven Oy
 * Copyright 2018 Confluent Inc.
+ * Copyright 2021 Meroxa, Inc.


Wrong year. Applies to all files where this line appears.

Ah, good catch. Written last year.: )

juha-aiven · 2022-02-07T07:15:33Z

Hey, I saw your comments, but apparently no new commits?

What about the more important question:

Would it make sense to include the support "globally" and then it would be a "user error" if his DB didn't support it?

hariso · 2022-02-07T11:09:42Z

Hey, I saw your comments, but apparently no new commits?

What about the more important question:

Would it make sense to include the support "globally" and then it would be a "user error" if his DB didn't support it?

Hey Juha!

Sorry for the lack of activity here, unfortunately I wasn't able to get to it. However, I think today/tomorrow I'll be done with the comments here.

The reason why I believed it might make sense for this to be turned off by default is because I simply wasn't sure what would make sense. Now that you mentioned MySQL (I simply forgot checking it), I do believe it should be the default.

Edit: I've just checked this: https://insights.stackoverflow.com/survey/2021#most-popular-technologies-database-prof. The top 4 mentioned here do support multi-row inserts.

juha-aiven

Left some very minor comments.

Is the idea to git rid of MultiInsertDialect, move the code to GenericDatabaseDialect and make all dbs support multi insert?

juha-aiven · 2022-02-09T11:02:46Z

src/main/java/io/aiven/connect/jdbc/sink/PreparedStatementBinder.java

                break;
            default:
                throw new AssertionError();

        }
-        statement.addBatch();
+        // in a multi-row insert, all records are a single item in the batch
+        if (!(insertMode == MULTI)) {


Perhaps?

if (insertMode != MULTI) {

juha-aiven · 2022-02-09T11:05:57Z

src/test/java/io/aiven/connect/jdbc/sink/BufferedRecordsTest.java

 import static org.mockito.Mockito.when;

 public class BufferedRecordsTest {

    private final SqliteHelper sqliteHelper = new SqliteHelper(getClass().getSimpleName());
+    private String dbUrl;


Could be final?

Hmm, now that you ask, I think yes. I put it into setUp(), since I thought sqliteHelper.sqliteUri() can be called only after sqliteHelper is actually set up, but that doesn't appear to be the case.

juha-aiven · 2022-02-09T11:19:31Z

src/test/java/io/aiven/connect/jdbc/sink/BufferedRecordsTest.java

+                connection);
+
+        final Schema schema = newPlanetSchema();
+        for (int i = 1; i <= 5; i++) {


Is the idea to create five rows, but since the batch size is two, there is not a flush for the last row?

That's correct. Basically, a test for the flushing logic too. I'll add a comment, since it looks like it's not that obvious.: )

hariso · 2022-02-09T11:36:31Z

Left some very minor comments.

Is the idea to git rid of MultiInsertDialect, move the code to GenericDatabaseDialect and make all dbs support multi insert?

That's correct. The multi-insert support is part of GenericDatabaseDialect, but I forgot deleting MultiInsertDialect. I'll do that now and address the other feedback you left.

Thanks a lot!

src/main/java/io/aiven/connect/jdbc/dialect/DatabaseDialect.java

src/main/java/io/aiven/connect/jdbc/dialect/GenericDatabaseDialect.java

hariso · 2022-02-09T13:30:20Z

OK, so for some my IntelliJ doesn't show Checkstyle errors anymore, and that's why I missed those above. I'll fix soon.

hariso · 2022-02-23T16:07:25Z

@akudiyar @juha-aiven I force-pushed the branch, to have the commits signed. When you have some time, it'd be great if you could check this PR again.: )

juha-aiven

Added some minor comments, let's address these and this PR should be ready.

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java

jeremytee97 · 2023-12-18T09:37:01Z

any idea on when redshift dialect would be supported?

hariso requested a review from a team as a code owner December 17, 2021 18:19

juha-aiven reviewed Jan 20, 2022

View reviewed changes

hariso requested a review from juha-aiven February 7, 2022 11:34

juha-aiven requested changes Feb 9, 2022

View reviewed changes

hariso requested a review from juha-aiven February 9, 2022 11:50

akudiyar suggested changes Feb 9, 2022

View reviewed changes

src/main/java/io/aiven/connect/jdbc/dialect/DatabaseDialect.java Outdated Show resolved Hide resolved

src/main/java/io/aiven/connect/jdbc/dialect/GenericDatabaseDialect.java Show resolved Hide resolved

hariso force-pushed the multi-row-inserts-pg branch from bc1908f to f466813 Compare February 23, 2022 12:56

hariso requested a review from a team as a code owner February 23, 2022 12:56

hariso mentioned this pull request Feb 23, 2022

Automatically split batch in large multi-row inserts #117

Open

akudiyar previously approved these changes Feb 23, 2022

View reviewed changes

Support multi-row inserts

2df3063

hariso dismissed akudiyar’s stale review via 2df3063 February 23, 2022 16:05

hariso force-pushed the multi-row-inserts-pg branch from ecd18b1 to 2df3063 Compare February 23, 2022 16:05

juha-aiven requested changes Feb 28, 2022

View reviewed changes

PR feedback

d3fcb30

hariso requested review from juha-aiven and removed request for a team March 2, 2022 10:29

juha-aiven approved these changes Mar 2, 2022

View reviewed changes

juha-aiven merged commit e5b2b14 into Aiven-Open:master Mar 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi-row inserts #107

Support multi-row inserts #107

hariso commented Dec 17, 2021 •

edited

Loading

hariso commented Jan 17, 2022

juha-aiven left a comment

juha-aiven Jan 20, 2022

hariso Jan 20, 2022

juha-aiven Jan 20, 2022

hariso Jan 20, 2022

juha-aiven Jan 20, 2022

hariso Jan 20, 2022

juha-aiven commented Feb 7, 2022

hariso commented Feb 7, 2022 •

edited

Loading

juha-aiven left a comment

juha-aiven Feb 9, 2022

hariso Feb 9, 2022

juha-aiven Feb 9, 2022

hariso Feb 9, 2022

juha-aiven Feb 9, 2022

hariso Feb 9, 2022

hariso commented Feb 9, 2022

hariso commented Feb 9, 2022

hariso commented Feb 23, 2022

juha-aiven left a comment

jeremytee97 commented Dec 18, 2023

Support multi-row inserts #107

Support multi-row inserts #107

Conversation

hariso commented Dec 17, 2021 • edited Loading

hariso commented Jan 17, 2022

juha-aiven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juha-aiven commented Feb 7, 2022

hariso commented Feb 7, 2022 • edited Loading

juha-aiven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hariso commented Feb 9, 2022

hariso commented Feb 9, 2022

hariso commented Feb 23, 2022

juha-aiven left a comment

Choose a reason for hiding this comment

jeremytee97 commented Dec 18, 2023

hariso commented Dec 17, 2021 •

edited

Loading

hariso commented Feb 7, 2022 •

edited

Loading