-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat: Add support for Explain feature #1852
Changes from all commits
1ea42dd
8c7a5b0
177f07f
242e351
053dd92
0677368
87069ed
436775b
5047608
f09c6e3
411ca77
f663865
4b7127d
e3614be
e125c42
af79b7b
7a1937a
d0c6abf
9f084ba
0f76847
daddd3e
e38e917
60ad94e
e2813ab
32fc9de
e57da64
b4101ad
c59e964
a767158
1a8fd85
50808f1
1f4aa32
0610cae
faaa7cd
dc9dab2
4177cdd
105d8a5
6162363
d58acc0
638ae76
07e521c
dd9e83e
55f1736
2d7649f
40b3f89
c272165
10f06e8
8695453
0b66c53
a08c293
9996f1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/* | ||
* Copyright 2022 Google LLC | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.google.cloud.spanner.connection; | ||
|
||
import com.google.cloud.spanner.ErrorCode; | ||
import com.google.cloud.spanner.SpannerExceptionFactory; | ||
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException; | ||
import com.google.cloud.spanner.connection.ClientSideStatementValueConverters.ExplainCommandConverter; | ||
import com.google.common.collect.ImmutableSet; | ||
import java.lang.reflect.Method; | ||
import java.util.Set; | ||
import java.util.regex.Matcher; | ||
|
||
/** Specific executor for the EXPLAIN statement for PostgreSQL. */ | ||
class ClientSideStatementExplainExecutor implements ClientSideStatementExecutor { | ||
private final ClientSideStatementImpl statement; | ||
private final Method method; | ||
private final ExplainCommandConverter converter; | ||
public static final Set<String> EXPLAIN_OPTIONS = | ||
ImmutableSet.of( | ||
"verbose", "costs", "settings", "buffers", "wal", "timing", "summary", "format"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From https://www.postgresql.org/docs/current/sql-explain.html, it looks like Please check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing this out. I've added the code for handling this |
||
|
||
ClientSideStatementExplainExecutor(ClientSideStatementImpl statement) throws CompileException { | ||
try { | ||
this.statement = statement; | ||
this.converter = new ExplainCommandConverter(); | ||
this.method = | ||
ConnectionStatementExecutor.class.getDeclaredMethod( | ||
statement.getMethodName(), converter.getParameterClass()); | ||
} catch (Exception e) { | ||
throw new CompileException(e, statement); | ||
} | ||
} | ||
|
||
@Override | ||
public StatementResult execute(ConnectionStatementExecutor connection, String sql) | ||
throws Exception { | ||
return (StatementResult) method.invoke(connection, getParameterValue(sql)); | ||
} | ||
|
||
String getParameterValue(String sql) { | ||
Matcher matcher = statement.getPattern().matcher(sql); | ||
if (matcher.find() && matcher.groupCount() >= 1) { | ||
String value = matcher.group(0); | ||
if (value != null) { | ||
String res = converter.convert(value.trim()); | ||
if (res != null) { | ||
return res; | ||
} | ||
throw SpannerExceptionFactory.newSpannerException( | ||
ErrorCode.INVALID_ARGUMENT, String.format("Invalid argument for EXPLAIN: %s", value)); | ||
} | ||
} | ||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,9 +59,13 @@ | |
import com.google.cloud.spanner.CommitResponse; | ||
import com.google.cloud.spanner.CommitStats; | ||
import com.google.cloud.spanner.Dialect; | ||
import com.google.cloud.spanner.ErrorCode; | ||
import com.google.cloud.spanner.Options.RpcPriority; | ||
import com.google.cloud.spanner.ReadContext.QueryAnalyzeMode; | ||
import com.google.cloud.spanner.ResultSet; | ||
import com.google.cloud.spanner.ResultSets; | ||
import com.google.cloud.spanner.SpannerExceptionFactory; | ||
import com.google.cloud.spanner.Statement; | ||
import com.google.cloud.spanner.Struct; | ||
import com.google.cloud.spanner.TimestampBound; | ||
import com.google.cloud.spanner.Type; | ||
|
@@ -71,8 +75,11 @@ | |
import com.google.common.base.Preconditions; | ||
import com.google.common.collect.ImmutableMap; | ||
import com.google.protobuf.Duration; | ||
import com.google.spanner.v1.PlanNode; | ||
import com.google.spanner.v1.QueryPlan; | ||
import com.google.spanner.v1.RequestOptions; | ||
import com.google.spanner.v1.RequestOptions.Priority; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.Map; | ||
import java.util.concurrent.TimeUnit; | ||
|
@@ -83,6 +90,7 @@ | |
* calls are then forwarded into a {@link Connection}. | ||
*/ | ||
class ConnectionStatementExecutorImpl implements ConnectionStatementExecutor { | ||
|
||
static final class StatementTimeoutGetter implements DurationValueGetter { | ||
private final Connection connection; | ||
|
||
|
@@ -442,4 +450,196 @@ public StatementResult statementShowRPCPriority() { | |
public StatementResult statementShowTransactionIsolationLevel() { | ||
return resultSet("transaction_isolation", "serializable", SHOW_TRANSACTION_ISOLATION_LEVEL); | ||
} | ||
|
||
private String processQueryPlan(PlanNode planNode) { | ||
StringBuilder planNodeDescription = new StringBuilder(" : { "); | ||
com.google.protobuf.Struct metadata = planNode.getMetadata(); | ||
|
||
for (String key : metadata.getFieldsMap().keySet()) { | ||
planNodeDescription | ||
.append(key) | ||
.append(" : ") | ||
.append(metadata.getFieldsMap().get(key).getStringValue()) | ||
.append(" , "); | ||
} | ||
String substring = planNodeDescription.substring(0, planNodeDescription.length() - 3); | ||
planNodeDescription.setLength(0); | ||
planNodeDescription.append(substring).append(" }"); | ||
|
||
return planNodeDescription.toString(); | ||
} | ||
|
||
private String processExecutionStats(PlanNode planNode) { | ||
StringBuilder executionStats = new StringBuilder(""); | ||
for (String key : planNode.getExecutionStats().getFieldsMap().keySet()) { | ||
executionStats.append(key).append(" : { "); | ||
com.google.protobuf.Struct value = | ||
planNode.getExecutionStats().getFieldsMap().get(key).getStructValue(); | ||
for (String newKey : value.getFieldsMap().keySet()) { | ||
String newValue = value.getFieldsMap().get(newKey).getStringValue(); | ||
executionStats.append(newKey).append(" : ").append(newValue).append(" , "); | ||
} | ||
String substring = executionStats.substring(0, executionStats.length() - 3); | ||
executionStats.setLength(0); | ||
executionStats.append(substring).append(" } , "); | ||
} | ||
String substring = executionStats.substring(0, executionStats.length() - 3); | ||
executionStats.setLength(0); | ||
executionStats.append(substring); | ||
return executionStats.toString(); | ||
} | ||
|
||
private StatementResult getStatementResultFromQueryPlan(QueryPlan queryPlan, boolean isAnalyze) { | ||
ArrayList<Struct> list = new ArrayList<>(); | ||
for (PlanNode planNode : queryPlan.getPlanNodesList()) { | ||
String planNodeDescription = planNode.getDisplayName(); | ||
String executionStats = ""; | ||
|
||
if (!planNode.getMetadata().toString().equalsIgnoreCase("")) { | ||
planNodeDescription += processQueryPlan(planNode); | ||
} | ||
|
||
if (!planNode.getShortRepresentation().toString().equalsIgnoreCase("")) { | ||
planNodeDescription += " : " + planNode.getShortRepresentation().getDescription(); | ||
} | ||
|
||
if (isAnalyze && !planNode.getExecutionStats().toString().equals("")) { | ||
executionStats = processExecutionStats(planNode); | ||
} | ||
Struct.Builder builder = Struct.newBuilder().set("QUERY PLAN").to(planNodeDescription); | ||
|
||
if (isAnalyze) { | ||
builder.set("EXECUTION STATS").to(executionStats); | ||
} | ||
list.add(builder.build()); | ||
} | ||
|
||
ResultSet resultSet; | ||
if (isAnalyze) { | ||
resultSet = | ||
ResultSets.forRows( | ||
Type.struct( | ||
StructField.of("QUERY PLAN", Type.string()), | ||
StructField.of("EXECUTION STATS", Type.string())), | ||
list); | ||
} else { | ||
resultSet = | ||
ResultSets.forRows(Type.struct(StructField.of("QUERY PLAN", Type.string())), list); | ||
} | ||
return StatementResultImpl.of(resultSet); | ||
} | ||
|
||
private StatementResult executeStatement(String sql, QueryAnalyzeMode queryAnalyzeMode) { | ||
Statement statement = Statement.newBuilder(sql).build(); | ||
try (ResultSet resultSet = getConnection().analyzeQuery(statement, queryAnalyzeMode)) { | ||
while (resultSet.next()) { | ||
// ResultSet.next() should return false in order to access the ResultSet.Stats | ||
} | ||
|
||
if (resultSet.getStats() != null && resultSet.getStats().getQueryPlan() != null) { | ||
return getStatementResultFromQueryPlan( | ||
resultSet.getStats().getQueryPlan(), queryAnalyzeMode.equals(QueryAnalyzeMode.PROFILE)); | ||
} | ||
} | ||
throw SpannerExceptionFactory.newSpannerException( | ||
ErrorCode.FAILED_PRECONDITION, String.format("Couldn't fetch stats for %s", sql)); | ||
} | ||
|
||
// This method removes parenthesis from the sql string assuming it is ending with the closing | ||
// parenthesis | ||
private String removeParenthesisAndTrim(String sql) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to add a small comment that this method can only be used for SQL strings that have already been trimmed for leading and trailing spaces. Otherwise it would go wrong for a string like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed |
||
sql = sql.trim(); | ||
if (sql.charAt(0) == '(') { | ||
sql = sql.substring(1, sql.length() - 1); | ||
} | ||
return sql.trim(); | ||
} | ||
|
||
/* | ||
* This method executes the given SQL string in either PLAN or PROFILE mode and returns | ||
* the query plan and execution stats (if necessary). | ||
* | ||
* The only additional option that is supported is ANALYZE. The method will throw a SpannerException | ||
* if it is invoked with a statement that includes any other options. | ||
* | ||
* If the SQL string has ANALYZE option, it will be executed in PROFILE mode and will return a resultset | ||
* with two String columns namely QUERY PLAN and EXECUTION STATS. | ||
* | ||
* If the sql string doesn't have any option, it will be executed in PLAN mode and will return a resultset | ||
* with one string column namely QUERY PLAN. | ||
*/ | ||
@Override | ||
public StatementResult statementExplain(String sql) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment on the intended support for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
if (sql == null) { | ||
throw SpannerExceptionFactory.newSpannerException( | ||
ErrorCode.INVALID_ARGUMENT, String.format("Invalid String with Explain")); | ||
} | ||
|
||
if (sql.charAt(0) == '(') { | ||
int index = sql.indexOf(')'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
if (index == -1) { | ||
throw SpannerExceptionFactory.newSpannerException( | ||
ErrorCode.INVALID_ARGUMENT, | ||
String.format("Missing closing parenthesis in the query: %s", sql)); | ||
} | ||
String options[] = sql.substring(1, index).split("\\s*,\\s*"); | ||
boolean isAnalyze = false, startAfterIndex = false; | ||
for (String option : options) { | ||
String optionExpression[] = option.trim().split("\\s+"); | ||
if (optionExpression.length >= 3) { | ||
isAnalyze = false; | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is correct. If we encounter a string that contains more than 2 words, we will assume that the options expression did not contain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are 3 strings inside parenthesis like "explain (foo bar foo, analyse true) select * from table", it will send "(foo bar foo, analyse true) select * from table" as the query which will eventually cause exception. Its just the error message will not be appropriate (it will be something like "(foo bar foo, analyse true) select * from table" is not a query). Writing code for a solution which distinguishes the error message will be a bit tricky. Because, for the cases like "explain (select * from table)", it will be difficult to identify if the string "(select * from table)" is some option with 3 strings or a valid query string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds reasonable to me. |
||
} else if (ClientSideStatementExplainExecutor.EXPLAIN_OPTIONS.contains( | ||
optionExpression[0].toLowerCase())) { | ||
throw SpannerExceptionFactory.newSpannerException( | ||
ErrorCode.UNIMPLEMENTED, | ||
String.format("%s is not implemented yet", optionExpression[0])); | ||
} else if (optionExpression[0].equalsIgnoreCase("analyse") | ||
|| optionExpression[0].equalsIgnoreCase("analyze")) { | ||
isAnalyze = true; | ||
} else { | ||
isAnalyze = false; | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also here: I think that an unknown/invalid option expression should cause a failure, not be assumed to mean 'analyze is not in the options expression'. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there will be an unknown option like "explain (foo) select * from table", then it will send "(foo) select * from table" as the query, which will eventually cause exception. Its just the error message will not be appropriate (it will be something like "(foo) select * from table" is not a query). Writing code for a solution which distinguishes the error message will be a bit tricky. Because, for the cases like "explain (select * from table)", it will be difficult to identify if the string "select" is an unknown option or a part of some query. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. |
||
} | ||
|
||
if (optionExpression.length == 2) { | ||
if (optionExpression[1].equalsIgnoreCase("false") | ||
|| optionExpression[1].equalsIgnoreCase("0") | ||
|| optionExpression[1].equalsIgnoreCase("off")) { | ||
isAnalyze = false; | ||
startAfterIndex = true; | ||
} else if (!(optionExpression[1].equalsIgnoreCase("true") | ||
|| optionExpression[1].equalsIgnoreCase("1") | ||
|| optionExpression[1].equalsIgnoreCase("on"))) { | ||
isAnalyze = false; | ||
break; | ||
} | ||
} | ||
} | ||
if (isAnalyze) { | ||
String newSql = removeParenthesisAndTrim(sql.substring(index + 1)); | ||
return executeStatement(newSql, QueryAnalyzeMode.PROFILE); | ||
} else if (startAfterIndex) { | ||
String newSql = removeParenthesisAndTrim(sql.substring(index + 1)); | ||
return executeStatement(newSql, QueryAnalyzeMode.PLAN); | ||
} else { | ||
return executeStatement(removeParenthesisAndTrim(sql), QueryAnalyzeMode.PLAN); | ||
} | ||
} else { | ||
String[] arr = sql.split("\\s+", 2); | ||
if (arr.length >= 2) { | ||
String option = arr[0].toLowerCase(); | ||
String statementToBeExplained = arr[1]; | ||
|
||
if (ClientSideStatementExplainExecutor.EXPLAIN_OPTIONS.contains(option)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Options can only be given inside parentheses, except for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the case of "explain costs select * from foo", it will throw an exception like "Costs is unimplemented" instead of "Costs should be in parenthesis" which for now will not cause any major trouble. If possible, can we can pick this up sometime later after merging this initial PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that sounds good to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ack |
||
throw SpannerExceptionFactory.newSpannerException( | ||
ErrorCode.UNIMPLEMENTED, String.format("%s is not implemented yet", option)); | ||
} else if (option.equals("analyze") || option.equals("analyse")) { | ||
return executeStatement( | ||
removeParenthesisAndTrim(statementToBeExplained), QueryAnalyzeMode.PROFILE); | ||
} | ||
} | ||
return executeStatement(sql, QueryAnalyzeMode.PLAN); | ||
} | ||
} | ||
} |
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.
Is this change needed? Or did this not work in the current implementation?
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.
DatabaseClientImplTest.testExecutePartitionedDmlWithQuery test was failing without this line. I took this change from your PR (feat: support analyzeUpdate #1867 )