adnanhemani commented on code in PR #1686:
URL: https://github.com/apache/polaris/pull/1686#discussion_r2114569028
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/models/ModelEntity.java:
##########
@@ -324,6 +353,16 @@ public static PolarisBaseEntity toEntity(ModelEntity
model) {
entity.setProperties(model.getProperties());
entity.setInternalProperties(model.getInternalProperties());
entity.setGrantRecordsVersion(model.getGrantRecordsVersion());
+
+ if (model.location != null) {
+ if (subType == PolarisEntitySubType.ICEBERG_TABLE
+ || entityType == PolarisEntityType.NAMESPACE) {
+ HashMap<String, String> properties = new
HashMap<>(entity.getPropertiesAsMap());
Review Comment:
`addProperty` already does this!
##########
extension/persistence/relational-jdbc/src/main/resources/h2/schema-v2.sql:
##########
@@ -0,0 +1,124 @@
+--
+-- 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.
+--
+
+CREATE SCHEMA IF NOT EXISTS POLARIS_SCHEMA;
+SET SCHEMA POLARIS_SCHEMA;
+
+CREATE TABLE IF NOT EXISTS version (
+ version_key VARCHAR PRIMARY KEY,
+ version_value INTEGER NOT NULL
+);
+
+MERGE INTO version (version_key, version_value)
+ KEY (version_key)
+ VALUES ('version', 2);
+
+-- H2 supports COMMENT, but some modes may ignore it
+COMMENT ON TABLE version IS 'the version of the JDBC schema in use';
+
+DROP TABLE IF EXISTS entities;
+CREATE TABLE IF NOT EXISTS entities (
+ realm_id TEXT NOT NULL,
+ catalog_id BIGINT NOT NULL,
+ id BIGINT NOT NULL,
+ parent_id BIGINT NOT NULL,
+ name TEXT NOT NULL,
+ entity_version INT NOT NULL,
+ type_code INT NOT NULL,
+ sub_type_code INT NOT NULL,
+ create_timestamp BIGINT NOT NULL,
+ drop_timestamp BIGINT NOT NULL,
+ purge_timestamp BIGINT NOT NULL,
+ to_purge_timestamp BIGINT NOT NULL,
+ last_update_timestamp BIGINT NOT NULL,
+ properties TEXT NOT NULL DEFAULT '{}',
+ internal_properties TEXT NOT NULL DEFAULT '{}',
+ grant_records_version INT NOT NULL,
+ LOCATION TEXT,
Review Comment:
follow lowercase nomenclature that was previously set?
##########
extension/persistence/relational-jdbc/src/test/java/org/apache/polaris/extension/persistence/relational/jdbc/QueryGeneratorTest.java:
##########
@@ -45,7 +45,7 @@ void testGenerateSelectQuery_withMapWhereClause() {
whereClause.put("name", "testEntity");
whereClause.put("entity_version", 1);
String expectedQuery =
- "SELECT entity_version, to_purge_timestamp, internal_properties,
catalog_id, purge_timestamp, sub_type_code, create_timestamp,
last_update_timestamp, parent_id, name, id, drop_timestamp, properties,
grant_records_version, type_code FROM POLARIS_SCHEMA.ENTITIES WHERE
entity_version = 1 AND name = 'testEntity'";
+ "SELECT entity_version, to_purge_timestamp, internal_properties,
catalog_id, purge_timestamp, sub_type_code, create_timestamp,
last_update_timestamp, parent_id, name, location, id, drop_timestamp,
properties, grant_records_version, type_code FROM POLARIS_SCHEMA.ENTITIES WHERE
entity_version = 1 AND name = 'testEntity'";
Review Comment:
Update after talking to @singhpk234: I think this idempotent due to Java
having the same amount of buckets for a HashMap when started and us adding the
exact same keys to the map. But in the future, we should change over to a
`TreeMap` or something similar, which will guarantee the ordering for us.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2168,26 +2207,48 @@ private void createTableLike(TableIdentifier
identifier, PolarisEntity entity) {
private void createTableLike(
TableIdentifier identifier, PolarisEntity entity,
PolarisResolvedPathWrapper resolvedParent) {
+ IcebergTableLikeEntity icebergTableLikeEntity =
IcebergTableLikeEntity.of(entity);
+ // Set / suffix
+ boolean requireTrailingSlash =
+ callContext
+ .getPolarisCallContext()
+ .getConfigurationStore()
+ .getConfiguration(
+ callContext.getPolarisCallContext(),
+ FeatureConfiguration.ADD_TRAILING_SLASH_TO_LOCATION);
+ if (requireTrailingSlash
+ && icebergTableLikeEntity.getBaseLocation() != null
+ && !icebergTableLikeEntity.getBaseLocation().endsWith("/")) {
+ icebergTableLikeEntity =
+ new IcebergTableLikeEntity.Builder(icebergTableLikeEntity)
+ .setBaseLocation(icebergTableLikeEntity.getBaseLocation() + "/")
+ .build();
+ }
+
// Make sure the metadata file is valid for our allowed locations.
- String metadataLocation =
IcebergTableLikeEntity.of(entity).getMetadataLocation();
+ String metadataLocation = icebergTableLikeEntity.getMetadataLocation();
validateLocationForTableLike(identifier, metadataLocation, resolvedParent);
List<PolarisEntity> catalogPath = resolvedParent.getRawFullPath();
- if (entity.getParentId() <= 0) {
+ if (icebergTableLikeEntity.getParentId() <= 0) {
// TODO: Validate catalogPath size is at least 1 for catalog entity?
- entity =
- new PolarisEntity.Builder(entity)
+ icebergTableLikeEntity =
+ new IcebergTableLikeEntity.Builder(icebergTableLikeEntity)
.setParentId(resolvedParent.getRawLeafEntity().getId())
.build();
}
- entity =
- new
PolarisEntity.Builder(entity).setCreateTimestamp(System.currentTimeMillis()).build();
+ icebergTableLikeEntity =
Review Comment:
nit: I believe there is a cleaner way to operate on this without having to
keep building the entity and then replacing it?
##########
extension/persistence/relational-jdbc/src/test/java/org/apache/polaris/extension/persistence/relational/jdbc/QueryGeneratorTest.java:
##########
@@ -45,7 +45,7 @@ void testGenerateSelectQuery_withMapWhereClause() {
whereClause.put("name", "testEntity");
whereClause.put("entity_version", 1);
String expectedQuery =
- "SELECT entity_version, to_purge_timestamp, internal_properties,
catalog_id, purge_timestamp, sub_type_code, create_timestamp,
last_update_timestamp, parent_id, name, id, drop_timestamp, properties,
grant_records_version, type_code FROM POLARIS_SCHEMA.ENTITIES WHERE
entity_version = 1 AND name = 'testEntity'";
+ "SELECT entity_version, to_purge_timestamp, internal_properties,
catalog_id, purge_timestamp, sub_type_code, create_timestamp,
last_update_timestamp, parent_id, name, location, id, drop_timestamp,
properties, grant_records_version, type_code FROM POLARIS_SCHEMA.ENTITIES WHERE
entity_version = 1 AND name = 'testEntity'";
Review Comment:
This may be irrelevant to the current PR - but why did `location` get added
in this position amongst the columns? I'm assuming that this behavior is at
least idempotent - but in `ModelEntity` it is at the last position, but when
the query generated here, it is not.
If I read the code correctly, the ordering comes the way it is due to the
`keySet()` iterator on the `HashMap` we create in the `toMap()` function. But
key ordering is not guaranteed in `HashMap`s...so I'm not sure how these
columns are idempotently ordered so far?
Any insights?
--
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
