gemini-code-assist[bot] commented on code in PR #35796:
URL: https://github.com/apache/beam/pull/35796#discussion_r2257813950
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -1829,8 +1845,8 @@ public static RetryConfiguration create(
}
/**
- * An interface used by the JdbcIO Write to set the parameters of the {@link
PreparedStatement}
- * used to setParameters into the database.
+ * An interface used by the JdbcIO {@link ReadAll} and {@link Write} to set
the parameters of the
+ * {@link PreparedStatement} used to setParameters into the database.
Review Comment:
The phrase "used to setParameters into the database" is a bit awkward. A
more concise and clearer description would improve readability.
```suggestion
* An interface used by the JdbcIO {@link ReadAll} and {@link Write}
transforms to set the
* parameters of a {@link PreparedStatement}.
```
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -355,6 +355,10 @@ public static ReadRows readRows() {
* Like {@link #read}, but executes multiple instances of the query
substituting each element of a
* {@link PCollection} as query parameters.
*
+ * <p>The substitution is configured via {@link
ReadAll#withParameterSetter}. Substitutions
+ * allowed by Jdbc API's {@link PreparedStatement} are supported. In
particular, read from
+ * multiple tables are not supported.
Review Comment:
The phrase "read from multiple tables are not supported" is slightly
ambiguous and grammatically incorrect. It could be misinterpreted as not
allowing joins on multiple tables within a single query. A more precise
explanation would clarify that parameterizing the table name itself is not
supported.
```suggestion
* allowed by the JDBC API's {@link PreparedStatement} are supported. In
particular, this does not
* support parameterizing the table name to read from a different table
for each input element.
```
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -1175,6 +1179,18 @@ public ReadAll<ParameterT, OutputT>
withQuery(ValueProvider<String> query) {
return toBuilder().setQuery(query).build();
}
+ /**
+ * Provide a {@link PreparedStatementSetter} to replace the query string
for each input element
+ * for ReadAll transform.
+ *
+ * <p>For example,
+ *
+ * <pre>{@code
+ * JdbcIO.<String, Row>readAll()
+ * .withQuery("select * from table where field == ?")
+ * .withParameterSetter((element, preparedStatement) ->
{preparedStatement.setString(1, element)})
+ * }</pre>
Review Comment:
The Javadoc for `withParameterSetter` has a couple of issues:
1. The description is misleading. It states that `PreparedStatementSetter`
is used to "replace the query string", but it actually sets the parameters on
an existing `PreparedStatement`.
2. The SQL in the example is invalid (`==` should be `=`).
3. The lambda in the example can be simplified by removing the curly braces.
I've provided a suggestion that corrects these points for better clarity and
accuracy.
```suggestion
* Sets the {@link PreparedStatementSetter} to set the parameters of the
query for each input
* element.
*
* <p>For example,
*
* <pre>{@code
* JdbcIO.<String, Row>readAll()
* .withQuery("select * from table where field = ?")
* .withParameterSetter((element, preparedStatement) ->
preparedStatement.setString(1, element))
* }</pre>
```
--
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]
