adutra commented on code in PR #1802:
URL: https://github.com/apache/polaris/pull/1802#discussion_r2125835690
##########
persistence/relational-jdbc/build.gradle.kts:
##########
@@ -34,6 +34,7 @@ dependencies {
compileOnly(libs.jakarta.inject.api)
implementation(libs.smallrye.common.annotation) // @Identifier
+ implementation(libs.postgresql)
Review Comment:
This is creating a hard dependency from this project to the Postgres JDBC
driver, FYI.
##########
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:
This is very inefficient; we don't need to collect the object's current
values, we only need the keys (column names).
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -205,9 +227,14 @@ public static String getTableName(@Nonnull Class<?>
entityClass) {
throw new IllegalArgumentException("Unsupported entity class: " +
entityClass.getName());
}
- // TODO: check if we want to make schema name configurable.
- tableName = "POLARIS_SCHEMA." + tableName;
+ return "POLARIS_SCHEMA." + tableName;
+ }
- return tableName;
+ private void checkInvalidColumns(Map<String, Object> entity, Map<String,
Object> whereClause) {
+ List<String> allColumns = new ArrayList<>(entity.keySet());
+ allColumns.add("realm_id");
+ if (!allColumns.containsAll(whereClause.keySet())) {
+ throw new IllegalArgumentException("Invalid query " +
whereClause.keySet());
+ }
}
Review Comment:
```suggestion
private void checkInvalidColumns(Map<String, Object> entity, Map<String,
Object> whereClause) {
if (!whereClause.isEmpty()) {
for (String key : whereClause.keySet()) {
if (!entity.containsKey(key) && !key.equals("realm_id")) {
throw new IllegalArgumentException("Invalid query column: " + key);
}
}
}
```
##########
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:
Not related to this change, but why do we need so many WHERE conditions? The
PK is `(realmId, id)`, so that should be sufficient.
##########
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 tend to agree with @dimas-b, SQL should be statically defined at compile
time for maximum security and efficiency.
It should be possible to achieve that by declaring a few constants in each
model class. e.g. for `ModelEntity`:
```java
public class ModelEntity implements Converter<PolarisBaseEntity> {
public static final String TABLE_NAME = "ENTITIES";
public static final List<String> PK_COLUMNS = List.of("realm_id", "id");
public static final List<String> ALL_COLUMNS =
List.of(
"realm_id",
"id",
"catalog_id",
"parent_id",
"type_code",
"name",
"entity_version",
"sub_type_code",
"create_timestamp",
"drop_timestamp",
"purge_timestamp",
"to_purge_timestamp",
"last_update_timestamp",
"properties",
"internal_properties",
"grant_records_version");
public static final String LOOKUP_QUERY =
"SELECT "
+ String.join(", ", ALL_COLUMNS)
+ " FROM "
+ TABLE_NAME
+ " WHERE "
+ String.join(" = ? AND ", PK_COLUMNS)
+ " = ?";
public static final String LIST_QUERY =
"SELECT "
+ String.join(", ", ALL_COLUMNS)
+ " FROM "
+ TABLE_NAME
+ " WHERE realm_id = ? AND catalog_id = ? AND parent_id = ? AND
type_code = ?";
public static void bindLookupQuery(PreparedStatement stmt, ModelEntity
entity, String realmId) throws SQLException {
stmt.setObject(1, realmId);
stmt.setObject(2, entity.getId());
}
public static void bindListQuery(PreparedStatement stmt, ModelEntity
entity, String realmId) throws SQLException {
stmt.setObject(1, realmId);
stmt.setObject(2, entity.getCatalogId());
stmt.setObject(3, entity.getParentId());
stmt.setObject(4, entity.getTypeCode());
}
...
```
--
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]
