singhpk234 commented on code in PR #1686:
URL: https://github.com/apache/polaris/pull/1686#discussion_r2114464119


##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -190,6 +210,30 @@ public static String generateWhereClause(@Nonnull 
Map<String, Object> whereClaus
     return !whereConditionsString.isEmpty() ? (" WHERE " + 
whereConditionsString) : "";
   }
 
+  @VisibleForTesting
+  public static String generateVersionQuery() {
+    return "SELECT version_value FROM POLARIS_SCHEMA.VERSION";
+  }
+
+  @VisibleForTesting
+  public static String generateOverlapQuery(String realmId, long parentId, 
String location) {
+    String[] components = location.split("/");
+    StringBuilder locationClauseBuilder = new StringBuilder();
+    StringBuilder pathBuilder = new StringBuilder();
+    for (String component : components) {
+      pathBuilder.append(component).append("/");
+      locationClauseBuilder.append(String.format(" OR location = '%s'", 
pathBuilder));
+    }
+    locationClauseBuilder.append(String.format(" OR location LIKE '%s%%'", 
location));
+    String query = "SELECT " + String.join(", ", new 
ModelEntity().toMap().keySet());
+
+    // TODO harden against realmId in this method and others
+    return query
+        + String.format(
+            " FROM POLARIS_SCHEMA.entities WHERE realm_id = '%s' AND parent_id 
= %d AND (1 = 2%s)",

Review Comment:
   would prefer if we can move infering `POLARIS_SCHEMA.entitites` to the 
common logic below.



##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -190,6 +210,30 @@ public static String generateWhereClause(@Nonnull 
Map<String, Object> whereClaus
     return !whereConditionsString.isEmpty() ? (" WHERE " + 
whereConditionsString) : "";
   }
 
+  @VisibleForTesting
+  public static String generateVersionQuery() {
+    return "SELECT version_value FROM POLARIS_SCHEMA.VERSION";
+  }
+
+  @VisibleForTesting
+  public static String generateOverlapQuery(String realmId, long parentId, 
String location) {
+    String[] components = location.split("/");
+    StringBuilder locationClauseBuilder = new StringBuilder();
+    StringBuilder pathBuilder = new StringBuilder();
+    for (String component : components) {
+      pathBuilder.append(component).append("/");
+      locationClauseBuilder.append(String.format(" OR location = '%s'", 
pathBuilder));
+    }
+    locationClauseBuilder.append(String.format(" OR location LIKE '%s%%'", 
location));
+    String query = "SELECT " + String.join(", ", new 
ModelEntity().toMap().keySet());
+
+    // TODO harden against realmId in this method and others
+    return query
+        + String.format(
+            " FROM POLARIS_SCHEMA.entities WHERE realm_id = '%s' AND parent_id 
= %d AND (1 = 2%s)",
+            realmId, parentId, locationClauseBuilder);
+  }

Review Comment:
   does this means we now don't need to load table from persistence and can 
just work directly without persistence.



##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -556,6 +560,47 @@ public boolean hasChildren(
     }
   }
 
+  private int loadVersion() {
+    String query = generateVersionQuery();
+    try {
+      List<SchemaVersion> schemaVersion =
+          datasourceOperations.executeSelect(query, new SchemaVersion());
+      if (schemaVersion == null || schemaVersion.size() != 1) {
+        throw new RuntimeException("Failed to retrieve schema version");
+      }
+      return schemaVersion.getFirst().getValue();
+    } catch (SQLException e) {
+      LOGGER.error("Failed to load schema version due to {}", e.getMessage(), 
e);
+      throw new RuntimeException("Failed to retrieve schema version", e);
+    }
+  }
+
+  /** {@inheritDoc} */
+  @Override
+  public Optional<Optional<String>> hasOverlappingSiblings(
+      @Nonnull PolarisCallContext callContext, long parentId, String location) 
{
+    if (this.version < 2) {
+      return Optional.empty();
+    }

Review Comment:
   we haven't release v1 yet, if we can get your pr in we might not require v2



-- 
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]

Reply via email to