feat: Add support for multiple CopyData messages #40

Vizerai · 2022-02-17T11:16:22Z

No description provided.

Vizerai · 2022-02-17T11:21:15Z

I will add some additional tests to check row splitting between multiple CopyData messages.

tinaspark

It's hard to me to track all the different things going on, why they depend on each other, and if all the changes are adequately tested. I'm seeing a few things here

Instead of parsing the CopyData payloads as they come in, put them all in a buffer and parse when we receive a CopyDone -- this looks reasonable to me and fits the goal of this PR
Move parseCommand from IntermediateStatement to StatementParser -- this looks reasonable but should probably in another PR
Error checking for COPY_IN in ControlMessage.java and some status handling in QueryMessage.java -- this should probably be in another PR

src/main/java/com/google/cloud/spanner/pgadapter/statements/CopyStatement.java

…atform/pgadapter into copydata

…dapter into copydata

Vizerai · 2022-02-18T08:50:05Z

I broke off the other changes into separate PRs.

src/main/java/com/google/cloud/spanner/pgadapter/statements/CopyStatement.java

src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java

olavloite · 2022-02-18T12:03:52Z

src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java

  }

+  @Test
+  public void testMultipleCopyDataMessages() throws Exception {}


I assume that this is a placeholder for a test that is coming in a next PR?

Updated now.

olavloite · 2022-02-21T09:49:50Z

I think the current build errors might be caused by b/217744569

olavloite · 2022-02-21T15:00:41Z

I think the current build errors might be caused by b/217744569

That assumption was wrong. It's caused by another issue.

tinaspark · 2022-02-22T14:48:09Z

src/main/java/com/google/cloud/spanner/pgadapter/utils/MutationWriter.java

+   * @return True if current payload will fit within COMMIT_LIMIT. This is only an estimate and the
+   *     actual commit size may still be rejected by Spanner.
+   */
+  private boolean payloadFitsInCurrentBatch() {


can we name this commitSizeIsWithinLimit to match the naming of mutationCountIsWithinLimit? Also because the batching has been removed

tinaspark · 2022-02-22T14:48:50Z

src/main/java/com/google/cloud/spanner/pgadapter/utils/MutationWriter.java

-      rollback(connectionHandler, payload);
-      long mutationCount = this.mutationCount + records.size() * records.get(0).size();
-      long commitSize = this.batchSize + payload.length;
+  public void addMutations(ConnectionHandler connectionHandler, byte[] payload) throws Exception {


can you name this addCopyData since we aren't doing anything with mutations yet

tinaspark · 2022-02-22T14:50:45Z

src/main/java/com/google/cloud/spanner/pgadapter/utils/MutationWriter.java

-    } else {
-      System.err.println("File " + unsuccessfulCopy.getName() + " already exists");
-    }
+    this.fileWriter = new FileWriter(unsuccessfulCopy, false);
  }

  public void writeToErrorFile(byte[] payload) throws IOException {


rename to: writeCopyDataToErrorFile

add a comment that this is used if there are problems while collecting CopyData messages or building the mutation list

tinaspark · 2022-02-22T14:52:43Z

src/main/java/com/google/cloud/spanner/pgadapter/utils/MutationWriter.java

    }
+    this.fileWriter.write(new String(payload, StandardCharsets.UTF_8).trim() + "\n");
  }

  public void writeMutationsToErrorFile() throws IOException {


add a comment that this is used if Spanner returns an error when we try to commit the mutations

actually, I'm thinking about this again and why are we writing the mutations to the file at all? The original intention of the error file was for the user to look through the copy data that they sent to PGAdapter, fix it, and try again. I think we should always be writing the payload data, not the mutation data

Changed to write the copy data instead.

src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java

…atform/pgadapter into copydata

…dapter into copydata

tinaspark · 2022-02-22T19:33:05Z

src/main/java/com/google/cloud/spanner/pgadapter/utils/MutationWriter.java

    if (this.fileWriter == null) {
      createErrorFile();
    }
    this.fileWriter.write(new String(payload, StandardCharsets.UTF_8).trim() + "\n");
  }

+  /**
+   * The list of mutaitons will be written to the error file if a problem was encountered while


Reworded comment.

tinaspark · 2022-02-22T19:36:29Z

src/main/java/com/google/cloud/spanner/pgadapter/utils/MutationWriter.java

    }
+    this.fileWriter.write(new String(payload, StandardCharsets.UTF_8).trim() + "\n");
  }

  public void writeMutationsToErrorFile() throws IOException {


actually, I'm thinking about this again and why are we writing the mutations to the file at all? The original intention of the error file was for the user to look through the copy data that they sent to PGAdapter, fix it, and try again. I think we should always be writing the payload data, not the mutation data

tinaspark · 2022-02-22T21:50:46Z

src/main/java/com/google/cloud/spanner/pgadapter/utils/MutationWriter.java

-    for (Mutation mutation : this.mutations) {
-      this.fileWriter.write(mutation.toString());
-    }
+  public void writeCopyDataToErrorFile() throws IOException {


I think every time we write to the error file we write the full payload so you can just have a single version of writeCopyDataToErrorFile() without having a payload argument

src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java

Adding support for multiple CopyData messages within a single Copy.

6c99e9b

Vizerai requested review from olavloite and tinaspark February 17, 2022 11:16

tinaspark requested changes Feb 17, 2022

View reviewed changes

src/main/java/com/google/cloud/spanner/pgadapter/statements/CopyStatement.java Outdated Show resolved Hide resolved

Vizerai added 8 commits February 17, 2022 14:37

Merge branch 'postgresql-dialect' into copydata

3bf04e2

Update

aa99530

Merge branch 'postgresql-dialect' of https://github.com/GoogleCloudPl…

4e38fc1

…atform/pgadapter into copydata

Merge branch 'copydata' of https://github.com/GoogleCloudPlatform/pga…

2eb1f3e

…dapter into copydata

Update

9a87f4d

Break changes into multiple PRs.

2f0dfbb

Update

483d553

Update

376028a

olavloite approved these changes Feb 18, 2022

View reviewed changes

Update

3690d1b

Vizerai changed the title ~~Adding support for multiple CopyData messages within a single Copy.~~ Adding support for multiple CopyData messages Feb 21, 2022

Vizerai requested a review from tinaspark February 21, 2022 01:00

Vizerai added 3 commits February 20, 2022 17:02

Update

9dc84c6

Update

98b38cd

Update

a337b21

Vizerai mentioned this pull request Feb 22, 2022

feat: Add Copy command parsing in QueryMessage and basic psql e2e test #43

Merged

olavloite changed the title ~~Adding support for multiple CopyData messages~~ feat: Add support for multiple CopyData messages Feb 22, 2022

Merge branch 'postgresql-dialect' into copydata

02750de

olavloite approved these changes Feb 22, 2022

View reviewed changes

tinaspark requested changes Feb 22, 2022

View reviewed changes

Vizerai added 2 commits February 22, 2022 11:24

Renaming methods and updating tests.

c7d9b96

Merge branch 'postgresql-dialect' of https://github.com/GoogleCloudPl…

dffd42f

…atform/pgadapter into copydata

Merge branch 'copydata' of https://github.com/GoogleCloudPlatform/pga…

4c1aceb

…dapter into copydata

Vizerai requested a review from tinaspark February 22, 2022 19:29

Update

1aa069e

tinaspark requested changes Feb 22, 2022

View reviewed changes

Update names/comments

27fe164

Vizerai requested a review from tinaspark February 22, 2022 21:23

tinaspark requested changes Feb 22, 2022

View reviewed changes

Update

52865b5

Vizerai requested a review from tinaspark February 22, 2022 22:09

tinaspark approved these changes Feb 22, 2022

View reviewed changes

Vizerai merged commit 24eeedc into postgresql-dialect Feb 22, 2022

olavloite deleted the copydata branch March 6, 2022 16:05

release-please bot mentioned this pull request Mar 18, 2022

chore(postgresql-dialect): release 0.1.0 #65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for multiple CopyData messages #40

feat: Add support for multiple CopyData messages #40

Vizerai commented Feb 17, 2022

Vizerai commented Feb 17, 2022

tinaspark left a comment

Vizerai commented Feb 18, 2022

olavloite Feb 18, 2022

Vizerai Feb 21, 2022

olavloite commented Feb 21, 2022

olavloite commented Feb 21, 2022

tinaspark Feb 22, 2022

Vizerai Feb 22, 2022

tinaspark Feb 22, 2022

Vizerai Feb 22, 2022

tinaspark Feb 22, 2022

Vizerai Feb 22, 2022

tinaspark Feb 22, 2022

Vizerai Feb 22, 2022

tinaspark Feb 22, 2022

Vizerai Feb 22, 2022

tinaspark Feb 22, 2022

Vizerai Feb 22, 2022

tinaspark Feb 22, 2022

tinaspark Feb 22, 2022

Vizerai Feb 22, 2022

feat: Add support for multiple CopyData messages #40

feat: Add support for multiple CopyData messages #40

Conversation

Vizerai commented Feb 17, 2022

Vizerai commented Feb 17, 2022

tinaspark left a comment

Choose a reason for hiding this comment

Vizerai commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented Feb 21, 2022

olavloite commented Feb 21, 2022

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

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

Choose a reason for hiding this comment