Spanner Persistence Backend for Polaris (Initial change) #3259

talatuyarer · 2025-12-12T18:31:26Z

Rebased @byronellis's Original PR #2328 against latest main branch.

Checklist

🛡️ Don't disclose security issues! (contact [email protected])
🔗 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)

dimas-b · 2025-12-12T18:33:42Z

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

talatuyarer · 2025-12-12T18:35:35Z

Yes This is my plan @dimas-b

dimas-b · 2025-12-12T18:42:31Z

@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)

dimas-b · 2025-12-12T19:03:35Z

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

Co-authored-by: Byron Ellis <[email protected]>

talatuyarer · 2025-12-12T19:25:54Z

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

byronellis · 2025-12-12T19:27:31Z

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

talatuyarer · 2025-12-12T19:31:19Z

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.

dimas-b

Just one concern about SchemaOptions (which was not affected by #2328), otherwise LGTM 👍

polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java

dimas-b

I'll leave the PR open for a couple more days since the original PR had comments from other people. I wonder if they may want to review again.

flyrain

Thanks a lot for rebasing it, @talatuyarer ! This is a good start point for Spanner persistence. Given this PR is a rebased one and the general direction is fine, I think it's OK to merge it as is, or with minor changes. We need to fine issues or and fix forward.

flyrain · 2025-12-13T23:21:21Z

gradle/libs.versions.toml

 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" }


+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: #2328 (comment)).

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.

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

flyrain · 2025-12-13T23:34:38Z

...er/src/main/java/org/apache/polaris/persistence/spanner/GoogleCloudSpannerConfiguration.java

+@ConfigMapping(prefix = "polaris.persistence.spanner")
+public interface GoogleCloudSpannerConfiguration {
+
+  public Optional<String> quotaProjectId();


minor: all methods are public by default in an interface, no need to explicitly add a modifier.

Suggested change

public Optional<String> quotaProjectId();

Optional<String> quotaProjectId();

flyrain · 2025-12-13T23:39:41Z

...org/apache/polaris/persistence/spanner/GoogleCloudSpannerDatabaseClientLifecycleManager.java

+import org.slf4j.LoggerFactory;
+
+@ApplicationScoped
+public class GoogleCloudSpannerDatabaseClientLifecycleManager {


Nit: Does it make sense to shorten the name a bit? I think SpannerClientLifecycleManager should be concise and descriptive enough here,

Suggested change

public class GoogleCloudSpannerDatabaseClientLifecycleManager {

public class SpannerClientLifecycleManager {

flyrain · 2025-12-13T23:40:37Z

...er/src/main/java/org/apache/polaris/persistence/spanner/GoogleCloudSpannerConfiguration.java

+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.persistence.spanner")
+public interface GoogleCloudSpannerConfiguration {


Nit: same naming suggestion here

Suggested change

public interface GoogleCloudSpannerConfiguration {

public interface SpannerConfiguration {

singhpk234

Thanks @talatuyarer for the contribution !

singhpk234 · 2025-12-14T01:14:18Z

persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql

+`Properties` JSON,
+InternalProperties JSON,


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

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

singhpk234 · 2025-12-14T01:18:18Z

persistence/google-cloud-spanner/src/main/resources/spanner/schema-v1.sql

@@ -0,0 +1,86 @@
+--


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 ?

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

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

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

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.

github-project-automation bot added this to Basic Kanban Board Dec 12, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Dec 12, 2025

dimas-b changed the title ~~Rebased Spanner Backend's Initial PR~~ Spanner Persistence Backend for Polaris (Initial change) Dec 12, 2025

talatuyarer and others added 2 commits December 12, 2025 11:14

Rebases Byron's PR agaist latest main

65d8c22

Co-authored-by: Byron Ellis <[email protected]>

Removed space

67a58c2

talatuyarer force-pushed the spanner-backend-rebase branch from a175461 to 67a58c2 Compare December 12, 2025 19:24

dimas-b reviewed Dec 12, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java Outdated Show resolved Hide resolved

dimas-b requested review from dennishuo, singhpk234 and snazy December 12, 2025 19:43

Removed schemaFile support and aligned recent schemaVersion change

3c7dc17

talatuyarer requested a review from dimas-b December 12, 2025 20:24

dimas-b approved these changes Dec 13, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Dec 13, 2025

flyrain approved these changes Dec 13, 2025

View reviewed changes

singhpk234 reviewed Dec 14, 2025

View reviewed changes

	public Optional<String> quotaProjectId();
	Optional<String> quotaProjectId();

	public class GoogleCloudSpannerDatabaseClientLifecycleManager {
	public class SpannerClientLifecycleManager {

	public interface GoogleCloudSpannerConfiguration {
	public interface SpannerConfiguration {

		`Properties` JSON,
		InternalProperties JSON,

Spanner Persistence Backend for Polaris (Initial change) #3259

Are you sure you want to change the base?

Spanner Persistence Backend for Polaris (Initial change) #3259

Uh oh!

Conversation

talatuyarer commented Dec 12, 2025

Checklist

Uh oh!

dimas-b commented Dec 12, 2025

Uh oh!

talatuyarer commented Dec 12, 2025

Uh oh!

dimas-b commented Dec 12, 2025

Uh oh!

dimas-b commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talatuyarer commented Dec 12, 2025

Uh oh!

byronellis commented Dec 12, 2025

Uh oh!

talatuyarer commented Dec 12, 2025

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dimas-b commented Dec 12, 2025 •

edited

Loading

flyrain Dec 13, 2025 •

edited

Loading