You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "talatuyarer (via GitHub)" <gi...@apache.org> on 2025/12/12 18:31:28 UTC

[PR] Rebased Spanner Backend's Initial PR [polaris]

talatuyarer opened a new pull request, #3259:
URL: https://github.com/apache/polaris/pull/3259

   Rebased @byronellis's Original PR #2328 against latest main branch. 
   
   cc @flyrain 
   
   ## Checklist
   - [ ] 🛡️ Don't disclose security issues! (contact security@apache.org)
   - [ ] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
   - [ ] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
   - [ ] 💡 Added comments for complex logic
   - [ ] 🧾 Updated `CHANGELOG.md` (if needed)
   - [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
   


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2616596853


##########
persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/spanner/GoogleCloudSpannerDatabaseClientLifecycleManager.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * 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.spanner;
+
+import com.google.cloud.spanner.Database;
+import com.google.cloud.spanner.DatabaseAdminClient;
+import com.google.cloud.spanner.DatabaseId;
+import com.google.cloud.spanner.Dialect;
+import com.google.cloud.spanner.Spanner;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Produces;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutionException;
+import org.apache.polaris.core.persistence.bootstrap.SchemaOptions;
+import org.apache.polaris.persistence.spanner.util.SpannerUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@ApplicationScoped
+public class GoogleCloudSpannerDatabaseClientLifecycleManager {

Review Comment:
   Nit: Does it make sense to shorten the name a bit? I think `SpannerClientLifecycleManager` should be concise and descriptive enough here,
   ```suggestion
   public class SpannerClientLifecycleManager {
   ```



##########
gradle/libs.versions.toml:
##########
@@ -52,8 +52,7 @@ cel-bom = { module = "org.projectnessie.cel:cel-bom", version = "0.5.3" }
 commons-lang3 = { module = "org.apache.commons:commons-lang3", version = "3.20.0" }
 commons-text = { module = "org.apache.commons:commons-text", version = "1.15.0" }
 errorprone = { module = "com.google.errorprone:error_prone_core", version = "2.45.0" }
-google-cloud-iamcredentials = { module = "com.google.cloud:google-cloud-iamcredentials", version = "2.80.0" }
-google-cloud-storage-bom = { module = "com.google.cloud:google-cloud-storage-bom", version = "2.60.0" }
+google-cloud-libraries-bom = { module = "com.google.cloud:libraries-bom", version = "26.72.0" }

Review Comment:
   +1, thanks for the change, this makes the versions more consistent across Google Cloud components.
   
   Continuing my review of #2328: a license update is still required. I’m fine with addressing this in a follow-up (see the earlier discussion here: https://github.com/apache/polaris/pull/2328#discussion_r2270902715).
   
   Would it make sense to file a tracking issue and mark it as a 1.4 release blocker? That would help streamline the release manager’s workflow, since any outstanding license issue will cause a release RC to fail.



##########
persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/spanner/GoogleCloudSpannerConfiguration.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.spanner;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.persistence.spanner")
+public interface GoogleCloudSpannerConfiguration {

Review Comment:
   Nit: same naming suggestion here
   ```suggestion
   public interface SpannerConfiguration {
   ```



##########
persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/spanner/GoogleCloudSpannerConfiguration.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.spanner;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.persistence.spanner")
+public interface GoogleCloudSpannerConfiguration {
+
+  public Optional<String> quotaProjectId();

Review Comment:
   minor: all methods are public in an interface.
   ```suggestion
    Optional<String> quotaProjectId();
   ```



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2620383009


##########
persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql:
##########
@@ -0,0 +1,86 @@
+--

Review Comment:
   Is Spanner schema related to JDBC schema in any way? Why should their respective version numbers be aligned?



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2615373971


##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -25,4 +25,6 @@
 @PolarisImmutable
 public interface SchemaOptions {
   Optional<Integer> schemaVersion();
+
+  Optional<String> schemaFile();

Review Comment:
   I'm not sure about this change. `schemaFile()` was present at some point, but got removed in #2762.
   
   Since this PR does not have any real call paths leading to it, would you mind deferring this change? I imagine (if it has to be done) the Admin tool will be affected too. WDYT?



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2615373971


##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -25,4 +25,6 @@
 @PolarisImmutable
 public interface SchemaOptions {
   Optional<Integer> schemaVersion();
+
+  Optional<String> schemaFile();

Review Comment:
   I'm not sure about this change. `schemaFile()` was present at some point, but got removed in #2762.
   
   Since this PR does not have a real call paths leading to it, would you mind deferring this change? I imaging (if it has to be done) the Admin tool will be affected too. WDYT?



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3647770488

   @talatuyarer : Since this is a rebase of the work previously submitted in #2328, WDYT about adding @byronellis as a co-author to these commits?


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2615373971


##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -25,4 +25,6 @@
 @PolarisImmutable
 public interface SchemaOptions {
   Optional<Integer> schemaVersion();
+
+  Optional<String> schemaFile();

Review Comment:
   I'm not sure about this change. `schemaFile()` was present at some point, but got removed in #2762.
   
   Since this PR does not have a real call paths leading to it, would you mind deferring this change? I imagine (if it has to be done) the Admin tool will be affected too. WDYT?



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "byronellis (via GitHub)" <gi...@apache.org>.
byronellis commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2625363026


##########
persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql:
##########
@@ -0,0 +1,86 @@
+--

Review Comment:
   Obviously inspired by, but no they're no real relation between the two (and has some Spanner-specific differences)



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3259: Spanner Persistence Backend for Polaris (Initial change)
URL: https://github.com/apache/polaris/pull/3259


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "byronellis (via GitHub)" <gi...@apache.org>.
byronellis commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3647853652

   Thanks for taking over Talat, sorry I haven't had a lot of time for it. FWIW here's the full implementation the first PR was based on. Looks like maybe a lot changed so not sure how relevant it is, but might be of some use: 
   
   https://github.com/byronellis/polaris/tree/spanner-persistence


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2615473904


##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -25,4 +25,6 @@
 @PolarisImmutable
 public interface SchemaOptions {
   Optional<Integer> schemaVersion();
+
+  Optional<String> schemaFile();

Review Comment:
   Thank you letting me know. I changed to code not use schemaFile. @dimas-b 



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2616631434


##########
persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql:
##########
@@ -0,0 +1,86 @@
+--

Review Comment:
   do we need spanner to also follow v1 of pg / h2 ? since we not using relational-jdbc way of implementing stuff should we just add v3 schema of relational-jdbc here wdyt ? 



##########
persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql:
##########
@@ -0,0 +1,86 @@
+--
+-- 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 SEQUENCE IF NOT EXISTS Ids OPTIONS(sequence_kind = 'bit_reversed_positive');
+
+CREATE TABLE IF NOT EXISTS Realms(
+  RealmId STRING(MAX) NOT NULL
+)
+PRIMARY KEY (RealmId);
+
+CREATE TABLE IF NOT EXISTS Entities(
+RealmId STRING(MAX) NOT NULL,
+CatalogId INT64 NOT NULL,
+Id INT64 NOT NULL,
+ParentId INT64 NOT NULL,
+Name STRING(MAX),
+EntityVersion INT64 NOT NULL,
+TypeCode INT64 NOT NULL,
+SubTypeCode INT64 NOT NULL,
+CreateTimestamp INT64,
+DropTimestamp INT64,
+PurgeTimestamp INT64,
+ToPurgeTimestamp INT64,
+LastUpdateTimestamp INT64,
+`Properties` JSON,
+InternalProperties JSON,

Review Comment:
   any reason we want JSON over JSONB seems like spanner supports it : https://docs.cloud.google.com/spanner/docs/working-with-jsonb



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2616702212


##########
persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql:
##########
@@ -0,0 +1,86 @@
+--

Review Comment:
   Following v3 schema of JDBC makes more sense to me, the Spanner impl. doesn't have to support the historical schemas.



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "byronellis (via GitHub)" <gi...@apache.org>.
byronellis commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2625360002


##########
persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql:
##########
@@ -0,0 +1,86 @@
+--
+-- 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 SEQUENCE IF NOT EXISTS Ids OPTIONS(sequence_kind = 'bit_reversed_positive');
+
+CREATE TABLE IF NOT EXISTS Realms(
+  RealmId STRING(MAX) NOT NULL
+)
+PRIMARY KEY (RealmId);
+
+CREATE TABLE IF NOT EXISTS Entities(
+RealmId STRING(MAX) NOT NULL,
+CatalogId INT64 NOT NULL,
+Id INT64 NOT NULL,
+ParentId INT64 NOT NULL,
+Name STRING(MAX),
+EntityVersion INT64 NOT NULL,
+TypeCode INT64 NOT NULL,
+SubTypeCode INT64 NOT NULL,
+CreateTimestamp INT64,
+DropTimestamp INT64,
+PurgeTimestamp INT64,
+ToPurgeTimestamp INT64,
+LastUpdateTimestamp INT64,
+`Properties` JSON,
+InternalProperties JSON,

Review Comment:
   Spanner didn't support JSONB in Spanner mode (just PostgreSQL compatibility mode) when this was originally written. :-)



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3647698852

   @talatuyarer : if you do not mind, please send an email to `dev` for awareness, since it's going to be a major new feature and people might have lost context (#2328 has been dormant since Sept)


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Rebased Spanner Backend's Initial PR [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3647668870

   @talatuyarer : Thanks for reviving Spanner Persistence! Do you intend to drive its implementation end-to-end (not just the first PR)?


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2616595110


##########
persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/spanner/GoogleCloudSpannerConfiguration.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.spanner;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.persistence.spanner")
+public interface GoogleCloudSpannerConfiguration {
+
+  public Optional<String> quotaProjectId();

Review Comment:
   minor: all methods are public by default in an interface, no need to explicitly add a modifier.
   ```suggestion
    Optional<String> quotaProjectId();
   ```



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2621323043


##########
gradle/libs.versions.toml:
##########
@@ -52,8 +52,7 @@ cel-bom = { module = "org.projectnessie.cel:cel-bom", version = "0.5.3" }
 commons-lang3 = { module = "org.apache.commons:commons-lang3", version = "3.20.0" }
 commons-text = { module = "org.apache.commons:commons-text", version = "1.15.0" }
 errorprone = { module = "com.google.errorprone:error_prone_core", version = "2.45.0" }
-google-cloud-iamcredentials = { module = "com.google.cloud:google-cloud-iamcredentials", version = "2.80.0" }
-google-cloud-storage-bom = { module = "com.google.cloud:google-cloud-storage-bom", version = "2.60.0" }
+google-cloud-libraries-bom = { module = "com.google.cloud:libraries-bom", version = "26.72.0" }

Review Comment:
   SGTM! I can create a follow up PR after merging this. 



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3647849276

   @dimas-b Make sense. i did not know co author feature. I added him as co author on the commit now. 


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2629094608


##########
persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql:
##########
@@ -0,0 +1,86 @@
+--

Review Comment:
   I think we should align the versions at least semantically. The schema directly dictates functionality. For example, without the new field in the entity table, the feature configuration `ALLOW_TABLE_LOCATION_OVERLAP` will not behave as expected. We should avoid a situation where a feature works with JDBC but fails with other persistence implementations due to schema drift.



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3857490534

   This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3716748765

   Hi @talatuyarer , would you mind resolving the conflicts so that we can merge the PR? 


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3647866628

   > Thanks for taking over Talat, sorry I haven't had a lot of time for it. FWIW here's the full implementation the first PR was based on. Looks like maybe a lot changed so not sure how relevant it is, but might be of some use:
   > 
   > https://github.com/byronellis/polaris/tree/spanner-persistence
   
   Thank you @byronellis Let me take a look that.


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Rebased Spanner Backend's Initial PR [polaris]

Posted by "talatuyarer (via GitHub)" <gi...@apache.org>.
talatuyarer commented on PR #3259:
URL: https://github.com/apache/polaris/pull/3259#issuecomment-3647676041

   Yes This is my plan @dimas-b 


-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2646374500


##########
gradle/libs.versions.toml:
##########
@@ -52,8 +52,7 @@ cel-bom = { module = "org.projectnessie.cel:cel-bom", version = "0.5.3" }
 commons-lang3 = { module = "org.apache.commons:commons-lang3", version = "3.20.0" }
 commons-text = { module = "org.apache.commons:commons-text", version = "1.15.0" }
 errorprone = { module = "com.google.errorprone:error_prone_core", version = "2.45.0" }
-google-cloud-iamcredentials = { module = "com.google.cloud:google-cloud-iamcredentials", version = "2.80.0" }
-google-cloud-storage-bom = { module = "com.google.cloud:google-cloud-storage-bom", version = "2.60.0" }
+google-cloud-libraries-bom = { module = "com.google.cloud:libraries-bom", version = "26.72.0" }

Review Comment:
   BOMs themselves do not cause transitive dependencies. They serve to align versions of individually listed dependencies coming from the same project. Unless individual dependencies are listed as `implementation`, etc., the BOM will not add any extra jars to Polaris.



-- 
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-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Spanner Persistence Backend for Polaris (Initial change) [polaris]

Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #3259:
URL: https://github.com/apache/polaris/pull/3259#discussion_r2646337777


##########
gradle/libs.versions.toml:
##########
@@ -52,8 +52,7 @@ cel-bom = { module = "org.projectnessie.cel:cel-bom", version = "0.5.3" }
 commons-lang3 = { module = "org.apache.commons:commons-lang3", version = "3.20.0" }
 commons-text = { module = "org.apache.commons:commons-text", version = "1.15.0" }
 errorprone = { module = "com.google.errorprone:error_prone_core", version = "2.45.0" }
-google-cloud-iamcredentials = { module = "com.google.cloud:google-cloud-iamcredentials", version = "2.80.0" }
-google-cloud-storage-bom = { module = "com.google.cloud:google-cloud-storage-bom", version = "2.60.0" }
+google-cloud-libraries-bom = { module = "com.google.cloud:libraries-bom", version = "26.72.0" }

Review Comment:
   I'm not sure if this is the same as with the AWS SDK, but using the BOM typically means larger shaded JARs if one is required for whatever reason. Is this a concern 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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org