eskabetxe commented on code in PR #160:
URL:
https://github.com/apache/flink-connector-jdbc/pull/160#discussion_r2084136580
##########
flink-connector-jdbc-core/src/main/java/org/apache/flink/connector/jdbc/core/table/source/JdbcDynamicTableSource.java:
##########
@@ -179,7 +179,7 @@ public ScanRuntimeProvider
getScanRuntimeProvider(ScanContext runtimeProviderCon
}
if (limit >= 0) {
- query = String.format("%s %s", query,
dialect.getLimitClause(limit));
+ query = dialect.addLimitClause(query, limit);
Review Comment:
The initial plan would be to deprecate the getLimitClause method in the
interface and keep it only in the AbstractDialect. This way, we don't have to
update all dialects or duplicate code between them.
Currently, getLimitClause is responsible for returning the part of the query
that handles the limit. Previously, adding this to the query was done in
JdbcDynamicTableSource, but now it has been moved to addLimitClause in
AbstractDialect, which always adds the limit at the end of the query. However,
in this case, the limit needs to be added at the beginning/middle of the query.
If the problem is with the method name, I don't see an issue with keeping it
the same as before. However, it does seem a bit odd to have a get method that
modifies a parameter passed to it.
Would you like to revisit the method naming or reconsider the overall
approach?
--
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]
