flyrain commented on code in PR #3584: URL: https://github.com/apache/polaris/pull/3584#discussion_r2771961030
########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelIdempotencyRecord.java: ########## @@ -0,0 +1,235 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.persistence.relational.jdbc.models; + +import jakarta.annotation.Nullable; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Timestamp; +import java.time.Instant; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.apache.polaris.core.entity.IdempotencyRecord; +import org.apache.polaris.immutables.PolarisImmutable; +import org.apache.polaris.persistence.relational.jdbc.DatabaseType; + +/** + * JDBC model for {@link IdempotencyRecord} mirroring the {@code idempotency_records} table. + * + * <p>This follows the same pattern as {@link ModelEvent}, separating the storage representation + * from the core domain model while still providing {@link Converter} helpers. + */ +@PolarisImmutable Review Comment: Do we need it to be PolarisImmutable? We didn't do that for `ModelEntity`, and a few others. ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java: ########## @@ -83,6 +83,11 @@ DatabaseType getDatabaseType() { return databaseType; } + /** Returns the detected database type for this datasource. */ + public DatabaseType databaseType() { Review Comment: Do we need this new method given that we've got a `getDatabaseType()` already(line 82)? We could change the scope to `public` if needed. ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java: ########## @@ -192,6 +192,65 @@ public static PreparedQuery generateUpdateQuery( return new PreparedQuery(sql, bindingParams); } + /** + * Builds an UPDATE query that updates only the specified columns and supports richer WHERE + * predicates (equality, greater-than, less-than, IS NULL, IS NOT NULL). + * + * <p>Callers should prefer passing an ordered map (e.g. {@link java.util.LinkedHashMap}) for the + * set clause so generated SQL and parameter order are stable. Review Comment: nit: feel a bit easier to understand to use `consistent`, or `matched`. Just my personal preference. Feel free to ignore it. ```suggestion * <p>Callers should prefer passing an ordered map (e.g. {@link java.util.LinkedHashMap}) for the * set clause so generated SQL and parameter order are consistent. ``` ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelIdempotencyRecord.java: ########## @@ -0,0 +1,235 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.persistence.relational.jdbc.models; + +import jakarta.annotation.Nullable; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Timestamp; +import java.time.Instant; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.apache.polaris.core.entity.IdempotencyRecord; +import org.apache.polaris.immutables.PolarisImmutable; +import org.apache.polaris.persistence.relational.jdbc.DatabaseType; + +/** + * JDBC model for {@link IdempotencyRecord} mirroring the {@code idempotency_records} table. + * + * <p>This follows the same pattern as {@link ModelEvent}, separating the storage representation + * from the core domain model while still providing {@link Converter} helpers. + */ +@PolarisImmutable +public interface ModelIdempotencyRecord extends Converter<IdempotencyRecord> { + + String TABLE_NAME = "idempotency_records"; + + // Logical tenant / realm identifier. + String REALM_ID = "realm_id"; + // Client-provided idempotency key. Review Comment: Nit: We got so many nice comments here while there is no comment on the class `IdempotencyRecord`. I'd love to have these comments on the class `IdempotencyRecord`. ########## polaris-core/src/main/java/org/apache/polaris/core/entity/IdempotencyRecord.java: ########## @@ -79,6 +86,14 @@ public String getOperationType() { return operationType; } + /** + * Normalized identifier of the resource affected by the operation. + * + * <p>This should be derived from the request (for example, a canonicalized path like {@code + * "tables/ns.tbl"}), not from a generated internal entity id. This ensures the binding is Review Comment: Do we define `canonicalized path` somewhere? Should we avoid duplication for a canonicalized path? A path like this `tables/ns.tbl` is very likely causing conflicts, as different catalogs could have the same namespace and table names. ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java: ########## @@ -209,6 +268,32 @@ public static PreparedQuery generateDeleteQuery( "DELETE FROM " + getFullyQualifiedTableName(tableName) + where.sql(), where.parameters()); } + /** + * Builds a DELETE query that supports richer WHERE predicates (equality, greater-than, less-than, + * IS NULL, IS NOT NULL). + */ + public static PreparedQuery generateDeleteQuery( + @Nonnull List<String> tableColumns, + @Nonnull String tableName, + @Nonnull Map<String, Object> whereEquals, + @Nonnull Map<String, Object> whereGreater, + @Nonnull Map<String, Object> whereLess, + @Nonnull Set<String> whereIsNull, + @Nonnull Set<String> whereIsNotNull) { + Set<String> columns = new HashSet<>(tableColumns); + validateColumns(columns, whereEquals.keySet()); + validateColumns(columns, whereGreater.keySet()); + validateColumns(columns, whereLess.keySet()); + validateColumns(columns, whereIsNull); + validateColumns(columns, whereIsNotNull); Review Comment: Do we need to validate them here as the method `generateWhereClauseExtended()` will validates them anyways? ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java: ########## @@ -231,22 +316,55 @@ static QueryFragment generateWhereClause( @Nonnull Set<String> tableColumns, @Nonnull Map<String, Object> whereEquals, @Nonnull Map<String, Object> whereGreater) { + return generateWhereClauseExtended( + tableColumns, whereEquals, whereGreater, Map.of(), Set.of(), Set.of()); + } + + private static void validateColumns( + @Nonnull Set<String> tableColumns, @Nonnull Set<String> columns) { + for (String column : columns) { + if (!tableColumns.contains(column) && !column.equals("realm_id")) { + throw new IllegalArgumentException("Invalid query column: " + column); Review Comment: `realm_id` is treated as a special implicit column for some models but explicitly included in `SELECT_COLUMNS` for `ModelIdempotencyRecord`. It might be cleaner to do something similar to other model, which keep `realm_id` out by only using `ALL_COLUMNS` instead of having `SELECT_COLUMNS`. WDYT? ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java: ########## @@ -231,22 +316,55 @@ static QueryFragment generateWhereClause( @Nonnull Set<String> tableColumns, @Nonnull Map<String, Object> whereEquals, @Nonnull Map<String, Object> whereGreater) { + return generateWhereClauseExtended( + tableColumns, whereEquals, whereGreater, Map.of(), Set.of(), Set.of()); + } + + private static void validateColumns( + @Nonnull Set<String> tableColumns, @Nonnull Set<String> columns) { + for (String column : columns) { + if (!tableColumns.contains(column) && !column.equals("realm_id")) { + throw new IllegalArgumentException("Invalid query column: " + column); + } + } + } + + @VisibleForTesting + static QueryFragment generateWhereClauseExtended( + @Nonnull Set<String> tableColumns, + @Nonnull Map<String, Object> whereEquals, + @Nonnull Map<String, Object> whereGreater, + @Nonnull Map<String, Object> whereLess, + @Nonnull Set<String> whereIsNull, + @Nonnull Set<String> whereIsNotNull) { + // Preserve the original behavior of rejecting unknown columns. This is used by SELECT query + // generation too, not only by callers of the extended UPDATE/DELETE helpers. Review Comment: Nit: I guess this comment is more like a dev log. Do we need them as a comment? ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java: ########## @@ -192,6 +192,65 @@ public static PreparedQuery generateUpdateQuery( return new PreparedQuery(sql, bindingParams); } + /** + * Builds an UPDATE query that updates only the specified columns and supports richer WHERE + * predicates (equality, greater-than, less-than, IS NULL, IS NOT NULL). + * + * <p>Callers should prefer passing an ordered map (e.g. {@link java.util.LinkedHashMap}) for the + * set clause so generated SQL and parameter order are stable. + * + * @param tableColumns List of valid table columns. + * @param tableName Target table. + * @param setClause Column-value pairs to update. + * @param whereEquals Column-value pairs used in WHERE equality filtering. + * @param whereGreater Column-value pairs used in WHERE greater-than filtering. + * @param whereLess Column-value pairs used in WHERE less-than filtering. + * @param whereIsNull Columns that must be NULL. + * @param whereIsNotNull Columns that must be NOT NULL. + * @return UPDATE query with parameter bindings. + */ + public static PreparedQuery generateUpdateQuery( + @Nonnull List<String> tableColumns, + @Nonnull String tableName, + @Nonnull Map<String, Object> setClause, + @Nonnull Map<String, Object> whereEquals, + @Nonnull Map<String, Object> whereGreater, + @Nonnull Map<String, Object> whereLess, + @Nonnull Set<String> whereIsNull, + @Nonnull Set<String> whereIsNotNull) { + if (setClause.isEmpty()) { + throw new IllegalArgumentException("Empty setClause"); + } + + Set<String> columns = new HashSet<>(tableColumns); + validateColumns(columns, setClause.keySet()); + validateColumns(columns, whereEquals.keySet()); + validateColumns(columns, whereGreater.keySet()); + validateColumns(columns, whereLess.keySet()); + validateColumns(columns, whereIsNull); + validateColumns(columns, whereIsNotNull); Review Comment: Same here -- 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]
