jiangzzhu commented on code in PR #40755:
URL: https://github.com/apache/beam/pull/40755#discussion_r4390416174
##########
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:
Evaluating an empty `composedPartitionTokens` list to `false` (0 rows) is
intentional and fixes a bug in the previous PostgreSQL implementation:
1. `getPartitionsMatchingState` is `private` and only called by:
- `updateToScheduled(List<String> composedPartitionTokens)` (which
iterates `for (String composedPartitionToken : composedPartitionTokens)` and
buffers 0 mutations when the list is empty), and
- `updateToRunning(String composedPartitionToken)` (which always passes
`Collections.singletonList(composedPartitionToken)`).
2. Previously, the `if (!composedPartitionTokens.isEmpty())` guard existed
because string-concatenating `= ANY (Array[])` without an explicit type cast
(`::text[]`) is a PostgreSQL syntax error (`cannot determine type of empty
array`). However, omitting the `"PartitionToken"` predicate caused
`updateToScheduled(emptyList)` to execute `SELECT * FROM "<metadataTable>"
WHERE "State" = 'CREATED'`, performing an unintended full-table scan and
acquiring read locks on all `CREATED` rows in the read-write transaction even
though none of them would be updated.
3. With a typed `Value.stringArray(...)` parameter bound to `$2`,
`"PartitionToken" = ANY ($2)` evaluates to `false` (0 rows) when
`composedPartitionTokens` is empty, aligning PostgreSQL with the GoogleSQL
branch right below (`WHERE PartitionToken IN UNNEST(@composedPartitionTokens)
AND State = @state`).
(Apologies for the confusion—the PR description previously had a wording
error claiming it omitted the filter when empty; I've fixed the PR description
wording as well.)
--
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]
