feat: Add support for Explain feature #1852

gauravsnj · 2022-04-25T06:18:54Z

Adds the support for the EXPLAIN, which will ultimately be utilised by PG Adapter

olavloite · 2022-04-25T06:50:45Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

+  public ResultSet statementExplain(String sql){
+
+    sql = sql.trim();
+    String firstString = sql.split(" ")[0].toLowerCase();


There are a couple of improvements that can be added to sql.split(..) here:

In this specific case, we only need the first word, so we should add a limit to the split invocation to prevent the entire string from being analyzed. Also; as we need the second keyword further below, it would be better to call split only once to get the two first keywords instead of calling split twice.

The keywords can be separated by any type of whitespace characters (so space, tab, linefeed, ...), and there could be more than one whitespace between keywords, so you should use split("\\s+")

Thanks for suggesting these improvements. I've added them in the code

olavloite · 2022-04-25T06:56:22Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

+    }
+
+    if(firstString.equals("analyze") || firstString.equals("analyse")) {
+      sql = sql.split(" ",2)[1];


Here also use "\\s+" to split the string, and consider combining the two calls to split into one.

...-spanner/src/main/resources/com/google/cloud/spanner/connection/PG_ClientSideStatements.json

olavloite · 2022-04-25T07:01:18Z

...anner/src/test/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorTest.java

@@ -237,4 +251,290 @@ public void testStatementSetPgTransactionModeNoOp() {
    verify(connection, never()).setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION);
    verify(connection, never()).setTransactionMode(TransactionMode.READ_WRITE_TRANSACTION);
  }
+
+  private ResultSet getMockResultSetForAnalyseQuery(){


Use Mockito to create a mock for this instead. See here for an example how to do that:

java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java

Line 196 in 12a0acb

ResultSet mockResultSet = mock(ResultSet.class);

olavloite · 2022-04-25T07:03:51Z

...anner/src/test/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorTest.java

+  public void testStatementExplain(){
+    String sql = "verbose select * from table";
+    try {
+      ResultSet rs = subject.statementExplain(sql);


Use Assert.assertThrows to verify that a method invocation throws a specific exception. See here for an example:

java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DialectTest.java

Line 93 in 5f156f2

assertThrows(IllegalArgumentException.class, () -> Dialect.fromName("INVALID"));

olavloite · 2022-04-25T07:22:29Z

...-spanner/src/main/resources/com/google/cloud/spanner/connection/PG_ClientSideStatements.json

@@ -133,6 +133,14 @@
      "method": "statementShowTransactionIsolationLevel",
      "exampleStatements": ["show transaction isolation level","show variable transaction isolation level"]
    },
+    {
+      "name": "EXPLAIN '<sql>'",
+      "executorName": "ClientSideStatementNoParamExecutor",


This won't work, as the method that you have defined has a parameter; the sql string that is to be explained is the input parameter in this case. You have two options for this:

You could reuse the ClientSideStatementSetExecutor for this. That executor is normally used for statements like SET <variable> = <value> where the value is the input parameter. In this case, the input parameter would be the entire sql string that follows EXPLAIN.

You could create a custom executor specifically for this method. See ClientSideStatementPgBeginExecutor for an example for that.

I think creating a new executor class will be better as ClientSideStatementSetExecutor is for specific type of commands and EXPLAIN clearly doesn't fit in that type. What do you say?

Yes, that would be the neatest solution for this.

…er/connection/PG_ClientSideStatements.json Co-authored-by: Knut Olav Løite <koloite@gmail.com>

…ature

olavloite

This is looking good! Please add more tests for corner cases like explain foo, explain, explain\nselect * from foo etc.

olavloite · 2022-04-25T17:18:41Z

...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java

+
+    @Override
+    public String convert(String value) {
+      return value.split(" +", 2)[1].toLowerCase();


A couple of points regarding this:

I think it should be programmed a little more defensively, so check that the array actually contains at least 2 items before trying to get the element at index 1, so we know that we won't run into any ArrayIndexOutOfBounds exceptions.

It should probably be value.split("\\s+"). That ensures that the statement is also correctly handled if there is a line feed or a tab between EXPLAIN and the rest of the statement. Please also add tests for that.

Why the .toLowerCase()? I don't think we need (or should) call that on the entire sql string. If we need it to determine whether the first token in the SQL string is one of the explain options, then we should only call toLowerCase() for that part. Otherwise, we will be sending a modified SQL string to the backend.

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

olavloite · 2022-04-25T17:20:45Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

+
+    sql = sql.trim();
+


olavloite · 2022-04-25T17:21:40Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

+    String[] arr = sql.split(" +", 2);
+
+    String option = arr[0];
+    String statementToBeExplained = arr[1];


Check to ensure that we did not just receive something like explain foo, which would cause an ArrayOutOfBoundsException here. Also add tests for that kind of corner cases.

…nnection/ConnectionStatementExecutorImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>

ansh0l

Have left a few comments, please take a look

ansh0l · 2022-04-27T15:16:55Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

@@ -442,4 +448,26 @@ public StatementResult statementShowRPCPriority() {
  public StatementResult statementShowTransactionIsolationLevel() {
    return resultSet("transaction_isolation", "serializable", SHOW_TRANSACTION_ISOLATION_LEVEL);
  }
+
+  @Override
+  public StatementResult statementExplain(String sql) {


Please add a comment on the intended support for Explain. Especially since there seem to be multiple permutations with EXPLAIN_OPTIONS.

ansh0l · 2022-04-27T15:19:32Z

...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementExplainExecutor.java

+  private final ExplainCommandConverter converter;
+  public static final Set<String> EXPLAIN_OPTIONS =
+      ImmutableSet.of(
+          "verbose", "costs", "settings", "buffers", "wal", "timing", "summary", "format");


From https://www.postgresql.org/docs/current/sql-explain.html, it looks like these keywords could be in uppercase. Are you doing any parsing etc to convert them to lowercase in input query?

Yes the parsing is being done before checking. Please check 548 and 512

ansh0l · 2022-04-27T15:21:33Z

...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementExplainExecutor.java

+          return res;
+        }
+        throw SpannerExceptionFactory.newSpannerException(
+            ErrorCode.INVALID_ARGUMENT, String.format("Unknown transaction mode: %s", value));


Since this is the Explain method, is Unknown transaction mode in ErrorCode the relevant message?

Changed the error message

...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java

ansh0l · 2022-04-27T15:26:26Z

...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementExplainExecutor.java

+  private final ExplainCommandConverter converter;
+  public static final Set<String> EXPLAIN_OPTIONS =
+      ImmutableSet.of(
+          "verbose", "costs", "settings", "buffers", "wal", "timing", "summary", "format");


From https://www.postgresql.org/docs/current/sql-explain.html, it looks like EXPLAIN (FORMAT JSON) SELECT * FROM foo; is a possible query, but I don't think its getting covered in your implementation.

Please check

Thanks for pointing this out. I've added the code for handling this

ansh0l · 2022-04-27T15:28:00Z

...anner/src/test/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorTest.java

+
+    ExplainCommandConverter converter = new ExplainCommandConverter();
+
+    /*Test for proper EXPLAIN*/


What does proper EXPLAIN mean? Are you referring for a valid EXPLAIN statement?

Yes it means valid explain.

ansh0l · 2022-04-27T15:28:46Z

...d-spanner/src/test/java/com/google/cloud/spanner/connection/ExplainCommandConverterTest.java

@@ -0,0 +1,58 @@
+/*
+ * Copyright 2019 Google LLC


2022 since this is a new file.

olavloite · 2022-05-12T18:07:53Z

...d-spanner/src/test/java/com/google/cloud/spanner/connection/ExplainCommandConverterTest.java

+
+@RunWith(Parameterized.class)
+public class ExplainCommandConverterTest {
+  @Parameter public Dialect dialect;


I think we can remove this parameter and make this an unparameterized test. We only support explain for PostgreSQL databases.

olavloite · 2022-05-12T18:08:32Z

...d-spanner/src/test/java/com/google/cloud/spanner/connection/ExplainCommandConverterTest.java

+  @Test
+  public void testConvert() throws CompileException {
+    ExplainCommandConverter explainCommandConverter = new ExplainCommandConverter();
+    Assert.assertEquals(


nit: add a static import for Assert.assertEquals to make the test case a little more compact

olavloite · 2022-05-12T18:09:38Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java

@@ -95,7 +95,7 @@ long executeStreamingPartitionedUpdate(
              resumeToken = rs.getResumeToken();
            }
            if (rs.hasStats()) {
-              foundStats = true;
+              foundStats = rs.getStats().hasRowCountLowerBound();


Is this change needed? Or did this not work in the current implementation?

DatabaseClientImplTest.testExecutePartitionedDmlWithQuery test was failing without this line. I took this change from your PR (feat: support analyzeUpdate #1867 )

olavloite · 2022-05-12T18:14:04Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

+        String arr2[] = probableOptions.split("\\s+");
+        if(arr2.length >= 3){
+          isAnalyse = false;
+          break;


That sounds reasonable to me.

olavloite · 2022-05-12T18:30:02Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

+    }
+
+    if (sql.charAt(0) == '(') {
+      int index = sql.indexOf(')');


This needs a quick check to verify that there actually is a closing parenthesis. If there is not, we should return a reasonable error message (something like 'missing closing parenthesis'), or otherwise just send the entire SQL string to the backend and let that determine what the error should be. Now it will probably fail with something like an IndexOutOfBoundsException, as index is -1.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ExplainTest.java

olavloite · 2022-05-12T21:22:21Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ExplainTest.java

+
+  @Test
+  public void testValidExplainWithFalseAnalyse() {
+    String statement = "    explain (analyse false)      " + EXPLAIN_STATEMENT_QUERY;


Here and elsewhere: PostgreSQL uses analyze instead of analyse in all documentation. I know both work, but I think it would be better if we also stuck to analyze (except for tests that verify that analyse also works).

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ExplainTest.java

This reverts commit 10f06e8.

olavloite · 2022-05-17T10:26:08Z

...e-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ITAbstractSpannerTest.java

-                .bind("table_name")
-                .to(table.toUpperCase())
+                    String.format(
+                        "SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES WHERE UPPER(TABLE_NAME)=UPPER(\'%s\')",


nit: We should never use string concatenation to add literals to a query, as it could be vulnerable to SQL injection attacks. It's not really relevant here, but it's good practice to always use statement parameters.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

ansh0l

LGTM with minor nits.

ansh0l · 2022-05-19T06:20:40Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

+        String option = arr[0].toLowerCase();
+        String statementToBeExplained = arr[1];
+
+        if (ClientSideStatementExplainExecutor.EXPLAIN_OPTIONS.contains(option)) {


...er/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java

gauravsnj added 3 commits April 21, 2022 21:07

Added the code for handling the explain

1ea42dd

Update ConnectionStatementExecutorImpl.java

8c7a5b0

Added tests

177f07f

gauravsnj requested review from a team as code owners April 25, 2022 06:18

product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Apr 25, 2022

Update PG_ClientSideStatements.json

242e351

gauravsnj requested a review from olavloite April 25, 2022 06:24

olavloite reviewed Apr 25, 2022

View reviewed changes

gauravsnj and others added 4 commits April 25, 2022 18:42

resolved the comments

053dd92

Update google-cloud-spanner/src/main/resources/com/google/cloud/spann…

0677368

…er/connection/PG_ClientSideStatements.json Co-authored-by: Knut Olav Løite <koloite@gmail.com>

Update ConnectionStatementExecutorTest.java

87069ed

Merge remote-tracking branch 'origin/explain-feature' into explain-fe…

436775b

…ature

product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Apr 25, 2022

gauravsnj added 3 commits April 25, 2022 20:09

Update PG_ClientSideStatements.json

5047608

Update PG_ClientSideStatements.json

f09c6e3

Formatted the files for ci lint

411ca77

olavloite reviewed Apr 25, 2022

View reviewed changes

gauravsnj and others added 5 commits April 26, 2022 07:40

Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/co…

f663865

…nnection/ConnectionStatementExecutorImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>

Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/co…

4b7127d

…nnection/ConnectionStatementExecutorImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>

resolved some comments

e3614be

formatted the code

e125c42

added some code

af79b7b

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 27, 2022

ansh0l suggested changes Apr 27, 2022

View reviewed changes

gauravsnj added 2 commits April 28, 2022 19:48

added support for "explain (format ) foo" kind of statements

7a1937a

resolved some comments

d0c6abf

product-auto-label bot added the size: xl Pull request size is extra large. label May 12, 2022

olavloite reviewed May 12, 2022

View reviewed changes

gauravsnj added 6 commits May 13, 2022 11:05

resolved comments

6162363

resolved comments

d58acc0

Update PG_ClientSideStatements.json

638ae76

Merge branch 'main' into explain-feature

07e521c

Update PG_ClientSideStatements.json

dd9e83e

Update PG_ClientSideStatements.json

55f1736

gauravsnj added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2022

gauravsnj added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels May 13, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 13, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2022

gauravsnj added 7 commits May 16, 2022 09:34

Create ITExplainTest.java

2d7649f

added Integration tests

40b3f89

reformatted

c272165

changed region

10f06e8

Revert "changed region"

8695453

This reverts commit 10f06e8.

Update ITExplainTest.java

0b66c53

Update ITExplainTest.java

a08c293

olavloite approved these changes May 17, 2022

View reviewed changes

olavloite requested a review from ansh0l May 17, 2022 10:30

olavloite added the owlbot:run Add this label to trigger the Owlbot post processor. label May 17, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 17, 2022

🦉 Updates from OwlBot post-processor

9996f1c

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

ansh0l approved these changes May 19, 2022

View reviewed changes

olavloite merged commit 01f460e into googleapis:main May 19, 2022

release-please bot mentioned this pull request May 19, 2022

chore(main): release 6.25.0 #1878

Merged

olavloite mentioned this pull request May 27, 2022

deps: update dependency com.google.cloud:google-cloud-spanner-bom to v6.25.2 googleapis/java-spanner-jdbc#881

Merged

1 task


		ExplainCommandConverter converter = new ExplainCommandConverter();

		/Test for proper EXPLAIN/

feat: Add support for Explain feature #1852

feat: Add support for Explain feature #1852

Conversation

gauravsnj commented Apr 25, 2022 • edited Loading

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

olavloite 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

ansh0l 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansh0l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravsnj commented Apr 25, 2022 •

edited

Loading