jiangzzhu commented on code in PR #40755:
URL: https://github.com/apache/beam/pull/40755#discussion_r4390324713
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/dao/PartitionMetadataDao.java:
##########
@@ -681,22 +690,20 @@ private Statement getPartitionsMatchingState(
List<String> composedPartitionTokens, State state) {
Statement statement;
if (this.dialect == Dialect.POSTGRESQL) {
- StringBuilder sqlStringBuilder =
- new StringBuilder("SELECT * FROM \"" + metadataTableName + "\"");
- sqlStringBuilder.append(" WHERE \"");
- sqlStringBuilder.append(COLUMN_STATE + "\" = " + "'" +
state.toString() + "'");
- if (!composedPartitionTokens.isEmpty()) {
- sqlStringBuilder.append(" AND \"");
- sqlStringBuilder.append(COLUMN_PARTITION_TOKEN);
- sqlStringBuilder.append("\"");
- sqlStringBuilder.append(" = ANY (Array[");
- sqlStringBuilder.append(
- composedPartitionTokens.stream()
- .map(s -> "'" + s + "'")
- .collect(Collectors.joining(",")));
- sqlStringBuilder.append("])");
- }
- statement = Statement.newBuilder(sqlStringBuilder.toString()).build();
+ statement =
+ Statement.newBuilder(
+ "SELECT * FROM \""
+ + metadataTableName
+ + "\" WHERE \""
+ + COLUMN_STATE
+ + "\" = $1 AND \""
+ + COLUMN_PARTITION_TOKEN
+ + "\" = ANY ($2)")
Review Comment:
Yeah, it seems change the behavior. However, this change may be safe
Here is why:
* Method semantics & all callers in PartitionMetadataDao.java:
getPartitionsMatchingState(List<String> composedPartitionTokens, State state)
is private and only has two callers:
1. updateToScheduled(List<String> composedPartitionTokens) (lines 535–557):
queries which tokens among composedPartitionTokens are currently in
State.CREATED, then loops for (String composedPartitionToken :
composedPartitionTokens) to update only those tokens to SCHEDULED. When
composedPartitionTokens is empty, that for loop executes 0 iterations and
updates nothing. Under the old PostgreSQL code, omitting AND "PartitionToken" =
ANY(...) ran SELECT * FROM "<metadataTable>" WHERE "State" = 'CREATED' —
performing an unnecessary full scan (and taking read locks on all CREATED rows
in the table inside a read-write transaction) only to discard every row
returned.
2. updateToRunning(String composedPartitionToken) (lines 564–582): always
passes Collections.singletonList(composedPartitionToken) (never empty).
* Matches the GoogleSQL branch (lines 707–722):
java
"SELECT * FROM " + metadataTableName
+ " WHERE " + COLUMN_PARTITION_TOKEN + " IN
UNNEST(@composedPartitionTokens) AND "
+ COLUMN_STATE + " = @state"
In GoogleSQL, passing an empty list binds an empty ARRAY<STRING> to
@composedPartitionTokens, and IN UNNEST([]) evaluates to false (0 rows).
The only reason the original PostgreSQL code had if
(!composedPartitionTokens.isEmpty()) was because it used raw SQL string
concatenation (= ANY (Array[...])), where an untyped empty literal Array[]
(without ::text[]) is a PostgreSQL syntax error (cannot determine type of empty
array).
With a typed bound Spanner parameter $2 (Value.stringArray(...)), Spanner
PostgreSQL resolves "PartitionToken" = ANY ($2) to the exact same
$in_array(STRING, ARRAY<STRING>) AST as GoogleSQL's IN
UNNEST(@composedPartitionTokens) (verified in parameterized_queries.test),
cleanly returning 0 rows when $2 is empty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
