dimas-b commented on code in PR #1802:
URL: https://github.com/apache/polaris/pull/1802#discussion_r2129078264
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -412,8 +464,10 @@ public <T> Page<T> listEntities(
// Limit can't be pushed down, due to client side filtering
// absence of transaction.
- String query = QueryGenerator.generateSelectQuery(new ModelEntity(),
params);
try {
+ PreparedQuery query =
+ queryGenerator.generateSelectQuery(
+ ModelEntity.ALL_COLUMNS, ModelEntity.TABLE_NAME, params);
Review Comment:
I believe it would be preferable to completely isolate SQL generation from
input values... E.g. use `params.keySet()` instead of all `params`.
Taking this one step further, since the key set is known for this use case,
we could have a specialized generator like `LookupEntitiesQuery.sql()`.
Currently, SQL specializations are contained in methods that call
`queryGenerator.generateSelectQuery()`. I'm proposing to make sub-classes for
those cases. I believe the amount of code is going to be roughly the same, but
we can achieve static SQL generation is most cases. In the remaining cases, I'm
sure we can still take runtime entities out of the SQL generation methods
(perhaps the only non-static case is going to be `IN` queries). WDYT?
--
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]
