-
Notifications
You must be signed in to change notification settings - Fork 132
feat: support unnamed parameters #3820
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
Changes from 4 commits
eebf2d0
29a5c5a
3f42072
899400c
d677799
cf7b397
fb618d5
07e5569
c8390b1
668be97
05713f3
0a7594c
1cd0c6e
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 |
---|---|---|
|
@@ -22,11 +22,16 @@ | |
import com.google.cloud.spanner.Options.UpdateOption; | ||
import com.google.cloud.spanner.SessionPool.PooledSessionFuture; | ||
import com.google.cloud.spanner.SpannerImpl.ClosedException; | ||
import com.google.cloud.spanner.Statement.StatementFactory; | ||
import com.google.common.annotations.VisibleForTesting; | ||
import com.google.common.base.Function; | ||
import com.google.common.util.concurrent.ListenableFuture; | ||
import com.google.spanner.v1.BatchWriteResponse; | ||
import io.opentelemetry.api.common.Attributes; | ||
import java.util.concurrent.ExecutionException; | ||
import java.util.concurrent.Future; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.TimeoutException; | ||
import javax.annotation.Nullable; | ||
|
||
class DatabaseClientImpl implements DatabaseClient { | ||
|
@@ -41,6 +46,8 @@ class DatabaseClientImpl implements DatabaseClient { | |
@VisibleForTesting final boolean useMultiplexedSessionPartitionedOps; | ||
@VisibleForTesting final boolean useMultiplexedSessionForRW; | ||
|
||
private StatementFactory statementFactory = null; | ||
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. nit: this can be removed 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. Done |
||
|
||
final boolean useMultiplexedSessionBlindWrite; | ||
|
||
@VisibleForTesting | ||
|
@@ -139,6 +146,21 @@ public Dialect getDialect() { | |
return pool.getDialect(); | ||
} | ||
|
||
@Override | ||
public StatementFactory newStatementFactory() { | ||
if (statementFactory == null) { | ||
sakthivelmanii marked this conversation as resolved.
Show resolved
Hide resolved
|
||
try { | ||
Dialect dialect = getDialectAsync().get(5, TimeUnit.SECONDS); | ||
sakthivelmanii marked this conversation as resolved.
Show resolved
Hide resolved
|
||
statementFactory = new StatementFactory(dialect); | ||
} catch (ExecutionException | TimeoutException e) { | ||
throw SpannerExceptionFactory.asSpannerException(e); | ||
} catch (InterruptedException e) { | ||
throw SpannerExceptionFactory.propagateInterrupt(e); | ||
} | ||
} | ||
return statementFactory; | ||
} | ||
|
||
@Override | ||
@Nullable | ||
public String getDatabaseRole() { | ||
|
@@ -346,6 +368,14 @@ public long executePartitionedUpdate(final Statement stmt, final UpdateOption... | |
return executePartitionedUpdateWithPooledSession(stmt, options); | ||
} | ||
|
||
private Future<Dialect> getDialectAsync() { | ||
MultiplexedSessionDatabaseClient client = getMultiplexedSessionDatabaseClient(); | ||
if (client != null) { | ||
return client.getDialectAsync(); | ||
} | ||
return pool.getDialectAsync(); | ||
} | ||
|
||
private long executePartitionedUpdateWithPooledSession( | ||
final Statement stmt, final UpdateOption... options) { | ||
ISpan span = tracer.spanBuilder(PARTITION_DML_TRANSACTION, commonAttributes); | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,118 @@ | ||||
/* | ||||
* Copyright 2025 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 | ||||
* | ||||
* https://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; | ||||
|
||||
import com.google.cloud.Date; | ||||
import com.google.protobuf.ListValue; | ||||
import java.text.SimpleDateFormat; | ||||
import java.time.LocalDate; | ||||
import java.time.LocalDateTime; | ||||
import java.time.OffsetDateTime; | ||||
import java.time.ZoneId; | ||||
import java.time.ZonedDateTime; | ||||
import java.time.format.DateTimeFormatter; | ||||
import java.time.temporal.TemporalAccessor; | ||||
import java.util.ArrayList; | ||||
import java.util.Iterator; | ||||
import java.util.List; | ||||
import java.util.function.Function; | ||||
import java.util.stream.Collectors; | ||||
import java.util.stream.Stream; | ||||
|
||||
final class SpannerTypeConverter { | ||||
|
||||
private static final String DATE_PATTERN = "yyyy-MM-dd"; | ||||
private static final SimpleDateFormat SIMPLE_DATE_FORMATTER = new SimpleDateFormat(DATE_PATTERN); | ||||
private static final ZoneId UTC_ZONE = ZoneId.of("UTC"); | ||||
private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern(DATE_PATTERN); | ||||
private static final DateTimeFormatter ISO_8601_DATE_FORMATTER = | ||||
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSX"); | ||||
|
||||
static <T> Value createUntypedArrayValue(Stream<T> stream) { | ||||
List<com.google.protobuf.Value> values = | ||||
stream | ||||
.map( | ||||
val -> | ||||
com.google.protobuf.Value.newBuilder() | ||||
.setStringValue(String.valueOf(val)) | ||||
.build()) | ||||
.collect(Collectors.toList()); | ||||
return Value.untyped( | ||||
com.google.protobuf.Value.newBuilder() | ||||
.setListValue(ListValue.newBuilder().addAllValues(values).build()) | ||||
.build()); | ||||
} | ||||
|
||||
static <T extends TemporalAccessor> String convertToISO8601(T dateTime) { | ||||
return ISO_8601_DATE_FORMATTER.format(dateTime); | ||||
} | ||||
|
||||
static <T> Value createUntypedValue(T value) { | ||||
sakthivelmanii marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
return Value.untyped( | ||||
com.google.protobuf.Value.newBuilder().setStringValue(String.valueOf(value)).build()); | ||||
} | ||||
|
||||
@SuppressWarnings("unchecked") | ||||
static <T, U> Iterable<U> convertToTypedIterable( | ||||
Function<T, U> func, T val, Iterator<?> iterator) { | ||||
List<U> values = new ArrayList<>(); | ||||
values.add(func.apply(val)); | ||||
iterator.forEachRemaining(value -> values.add(func.apply((T) value))); | ||||
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. Why do we need to do this in two separate steps (one for the first item, and then for each remaining item)? 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. because we are accessed the first element in the iterator to identify the type. |
||||
return values; | ||||
} | ||||
|
||||
static <T> Iterable<T> convertToTypedIterable(T val, Iterator<?> iterator) { | ||||
olavloite marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
return convertToTypedIterable(v -> v, val, iterator); | ||||
} | ||||
|
||||
static Date convertUtilDateToSpannerDate(java.util.Date date) { | ||||
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 is already a 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. done |
||||
return Date.parseDate(SIMPLE_DATE_FORMATTER.format(date)); | ||||
} | ||||
|
||||
static Date convertLocalDateToSpannerDate(LocalDate date) { | ||||
return Date.parseDate(DATE_FORMATTER.format(date)); | ||||
sakthivelmanii marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} | ||||
|
||||
static <T> Value createUntypedIterableValue( | ||||
T value, Iterator<?> iterator, Function<T, String> func) { | ||||
return Value.untyped( | ||||
com.google.protobuf.Value.newBuilder() | ||||
.setListValue( | ||||
ListValue.newBuilder() | ||||
.addAllValues( | ||||
SpannerTypeConverter.convertToTypedIterable( | ||||
(val) -> | ||||
com.google.protobuf.Value.newBuilder() | ||||
.setStringValue(func.apply(val)) | ||||
.build(), | ||||
value, | ||||
iterator))) | ||||
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 that this block could be optimized a bit by doing a direct conversion from the iterator straight into a ListValueBuilder, similar to how we are doing that here: Line 231 in 92494d0
The current implementation first creates a new ArrayList from the Iterator, and then adds the ArrayList to the ListValueBuilder. 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. Done |
||||
.build()); | ||||
} | ||||
|
||||
static ZonedDateTime convertToUTCTimezone(LocalDateTime localDateTime) { | ||||
sakthivelmanii marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
return localDateTime.atZone(UTC_ZONE); | ||||
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. Would it not make more sense to interpret this value with the system default timezone? 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. Done |
||||
} | ||||
|
||||
static ZonedDateTime convertToUTCTimezone(OffsetDateTime localDateTime) { | ||||
return localDateTime.atZoneSameInstant(UTC_ZONE); | ||||
} | ||||
|
||||
static ZonedDateTime convertToUTCTimezone(ZonedDateTime localDateTime) { | ||||
return localDateTime.withZoneSameInstant(UTC_ZONE); | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -20,6 +20,8 @@ | |||||
import static com.google.common.base.Preconditions.checkState; | ||||||
|
||||||
import com.google.cloud.spanner.ReadContext.QueryAnalyzeMode; | ||||||
import com.google.cloud.spanner.connection.AbstractStatementParser; | ||||||
import com.google.cloud.spanner.connection.AbstractStatementParser.ParametersInfo; | ||||||
import com.google.common.base.Preconditions; | ||||||
import com.google.common.collect.ImmutableMap; | ||||||
import com.google.spanner.v1.ExecuteSqlRequest.QueryOptions; | ||||||
|
@@ -246,4 +248,56 @@ StringBuilder toString(StringBuilder b) { | |||||
} | ||||||
return b; | ||||||
} | ||||||
|
||||||
public static final class StatementFactory { | ||||||
olavloite marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
private final Dialect dialect; | ||||||
|
||||||
StatementFactory(Dialect dialect) { | ||||||
this.dialect = dialect; | ||||||
} | ||||||
|
||||||
public Statement of(String sql) { | ||||||
return Statement.of(sql); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param sql SQL statement with unnamed parameter denoted 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.
Suggested change
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. Done |
||||||
* @param values list of values which needs to replace ? in the 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.
Suggested change
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. Done |
||||||
* @return Statement object | ||||||
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. Either remove the
Suggested change
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. Done |
||||||
* <p>This function accepts the SQL statement with unnamed parameters(?) and accepts the | ||||||
* list of objects to replace unnamed parameters. Primitive types are supported | ||||||
* <p>For Date column, following types are supported | ||||||
* <ul> | ||||||
* <li>java.util.Date | ||||||
* <li>LocalDate | ||||||
* <li>com.google.cloud.Date | ||||||
* </ul> | ||||||
* <p>For Timestamp column, following types are supported. All the dates should be in UTC | ||||||
* format. Incase if the timezone is not in UTC, spanner client will convert that to UTC | ||||||
* automatically | ||||||
* <ul> | ||||||
* <li>LocalDateTime | ||||||
* <li>OffsetDateTime | ||||||
* <li>ZonedDateTime | ||||||
* </ul> | ||||||
* <p> | ||||||
sakthivelmanii marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* @see DatabaseClient#newStatementFactory | ||||||
*/ | ||||||
public Statement of(String sql, Object... values) { | ||||||
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 that it would be good to clearly call out what the purpose is of this method, also in the method name.
Suggested change
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. Done |
||||||
Map<String, Value> parameters = getUnnamedParametersMap(values); | ||||||
AbstractStatementParser statementParser = AbstractStatementParser.getInstance(this.dialect); | ||||||
ParametersInfo parametersInfo = | ||||||
statementParser.convertPositionalParametersToNamedParameters('?', sql); | ||||||
return new Statement(parametersInfo.sqlWithNamedParameters, parameters, null); | ||||||
} | ||||||
|
||||||
private Map<String, Value> getUnnamedParametersMap(Object[] values) { | ||||||
Map<String, Value> parameters = new HashMap<>(); | ||||||
int index = 1; | ||||||
for (Object value : values) { | ||||||
parameters.put("p" + (index++), Value.toValue(value)); | ||||||
} | ||||||
return parameters; | ||||||
} | ||||||
} | ||||||
} |
Uh oh!
There was an error while loading. Please reload this page.