singhpk234 commented on code in PR #1802:
URL: https://github.com/apache/polaris/pull/1802#discussion_r2126914505
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -34,38 +37,67 @@
import org.apache.polaris.persistence.relational.jdbc.models.ModelGrantRecord;
import
org.apache.polaris.persistence.relational.jdbc.models.ModelPolicyMappingRecord;
import
org.apache.polaris.persistence.relational.jdbc.models.ModelPrincipalAuthenticationData;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class QueryGenerator {
+ private static final Logger log =
LoggerFactory.getLogger(QueryGenerator.class);
+ private final DatabaseType databaseType;
- public static <T> String generateSelectQuery(
+ public QueryGenerator(DatabaseType databaseType) {
+ this.databaseType = databaseType;
+ }
+
+ public DatabaseType getDatabaseType() {
+ return databaseType;
+ }
+
+ public static class PreparedQuery {
+ private final String sql;
+ private final List<Object> parameters;
+
+ public PreparedQuery(String sql, List<Object> parameters) {
+ this.sql = sql;
+ this.parameters = parameters;
+ }
+
+ public String getSql() {
+ return sql;
+ }
+
+ public List<Object> getParameters() {
+ return parameters;
+ }
+ }
+
+ public <T> PreparedQuery generateSelectQuery(
@Nonnull Converter<T> entity, @Nonnull Map<String, Object> whereClause) {
- return generateSelectQuery(entity, generateWhereClause(whereClause));
+
+ String tableName = getTableName(entity.getClass());
+ Map<String, Object> objectMap = entity.toMap(databaseType);
Review Comment:
I agree on this being a slight over kill, i was using reflection to get the
column names of the model, but there was a push back for that as well, other
way was to create a column_list for each model, its just duplication, i can
introduce a constant list, though I honestly don't think this is gonna be that
much of penalty since these are short lived objects and i expect, best is i can
define a method in Converter interface and call toMap() once and then cache the
result and keep sending it back, wdyt ?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -37,35 +40,51 @@
public class QueryGenerator {
- public static <T> String generateSelectQuery(
+ public static class PreparedQuery {
+ private final String sql;
+ private final List<Object> parameters;
+
+ public PreparedQuery(String sql, List<Object> parameters) {
+ this.sql = sql;
+ this.parameters = parameters;
+ }
+
+ public String getSql() {
+ return sql;
+ }
+
+ public List<Object> getParameters() {
+ return parameters;
+ }
+ }
+
+ public static <T> PreparedQuery generateSelectQuery(
@Nonnull Converter<T> entity, @Nonnull Map<String, Object> whereClause) {
Review Comment:
I agree with static SQL generation, though I would also request, to take up
avoid generating the SQL at run-time as fast follow-up, this requires some more
work and discussion since if now the QueryGenerator is shared across models and
some things like INSERT, DELETE are all in common place, never the less the
DBOperations class is not aware of what to bind the PreparedStatement against,
the parameters to bind is also decided in the QueryGenerator itself, all the
DbOps sees a PreparedStatement and Parameters and it opens preparedStatement
and feeds both stuff in, now if we go this ways we need to make DbOperation
understand what type of query it is and what to bind against. I am still
thinking of clean way to do this.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -269,8 +291,7 @@ public PolarisBaseEntity lookupEntity(
@Nonnull PolarisCallContext callCtx, long catalogId, long entityId, int
typeCode) {
Map<String, Object> params =
Map.of("catalog_id", catalogId, "id", entityId, "type_code", typeCode,
"realm_id", realmId);
- String query = generateSelectQuery(new ModelEntity(), params);
- return getPolarisBaseEntity(query);
+ return getPolarisBaseEntity(queryGenerator.generateSelectQuery(new
ModelEntity(), params));
Review Comment:
This should not be required ideally i am just honouring the params being
sent, since the interface defines it, presently yes this should not be required
give our PK. I think Eclipselink still requires (id, catalog_id) as PK, let me
know if you want me to skip this, i can remove this, its just there to honour
the params
--
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
