singhpk234 commented on code in PR #1802:
URL: https://github.com/apache/polaris/pull/1802#discussion_r2129138331
##########
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.
I agree with you, but presently, we just use params.keySet even today for
query generation the user values are just feed to the PreparedQuery record's
parameter, i can think off, specially since we assert the keys in params are
strict subset of the columns of the table. checking if i can stream line it
more and do the PreparedQuery in JDBC persistence it self
> Taking this one step further, since the key set is known for this use
case, we could have a specialized generator like LookupEntitiesQuery.sql().
There are 33 sql's presently, I agree static would be preferable, I am
parallely trying to do the same as well, here is a gist in which i have drilled
it down into all static queries late last night
https://gist.github.com/singhpk234/a63e7236a570892fcaf46bbf4cb89f34 except IN
(?) but i haven't tested it E2E, would need some more time for that, just
putting this pr out to address immediate concerns.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
