Spanner schema admin functions for parent-child relationships #910

ChengyuanZhao · 2018-07-28T02:50:30Z

related #853

# Conflicts: # spring-cloud-gcp-data-spanner/src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

meltsufin · 2018-07-30T12:03:41Z

...ava/org/springframework/cloud/gcp/data/spanner/core/mapping/SpannerPersistentEntityImpl.java

+						if (ConversionUtils.isIterableNonByteArrayType(
+								spannerPersistentProperty.getType())) {
+							throw new SpannerDataException(
+									"Embedded properties cannot be collections: "


Is it a limitation of Spanner, or is it just to disambiguate with one-to-many relationship?

EDIT, was answering a question about something else !

So no, this is a check to make sure people don't put a List<EmbeddedObject> with the annotation @Embedded because embedded objects are meant to be "flattened" into the containing class (columns merged with those of parents). It wouldn't make sense to flatten a list of that embedded object.

dmitry-s · 2018-07-30T15:45:22Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

+						"Could not find suitable Cloud Spanner column inner type for "
+								+ "property type: " + innerType);
+			}
+			ddlString = getTypeDdlString(spannerSupportedInnerType, true,


maybe just return the result here? if you do that, the further code can be refactored to avoid using else

ChengyuanZhao · 2018-08-02T17:17:43Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

+					spannerPersistentProperty.getMaxColumnLength());
+	}
+
+	private void getCreateTableDdlStringsForHierarchy(String parentTable,


More explicitly name to say "interleaved"

ChengyuanZhao

In-person review notes/changes

ChengyuanZhao · 2018-08-02T17:22:39Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

+						spannerPersistentEntity.tableName(),
+						spannerPersistentProperty.getColumnInnerType(), ddlStrings,
+						seenClasses));
+		seenClasses.add(entityClass);


need to add this to seen before recursion call

add test case where two properties have the same inner type to catch this

ChengyuanZhao · 2018-08-02T17:24:09Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

-	public String getDropTableDdlString(Class entityClass) {
-		return "DROP TABLE "
-				+ this.mappingContext.getPersistentEntity(entityClass).tableName();
+	private void getDropTableDdlStringsForHierarchy(Class entityClass,


drop DDL and create DDL is very similar, should refactor

ChengyuanZhao · 2018-08-02T17:30:18Z

...test/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtilsTests.java

+		@Column(name = "")
+		String other;
+
+		@OneToMany


Perhaps @Interleaved

ChengyuanZhao · 2018-08-02T17:31:07Z

...test/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtilsTests.java

+	public void getCreateDdlHierarchyTest() {
+		List<String> createStrings = this.spannerSchemaUtils
+				.getCreateTableDdlStringsForHierarchy(ParentEntity.class);
+		assertEquals(3, createStrings.size());


assert equals on array of all 3 strings?

meltsufin · 2018-08-02T17:20:45Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

+			return;
+		}
+		ddlStrings.add(getCreateTableDdlString(entityClass) + (parentTable == null ? ""
+				: ", INTERLEAVE IN PARENT " + parentTable + " ON DELETE CASCADE"));


Should ON DELETE CASCADE an option through a property?

ChengyuanZhao · 2018-08-02T18:54:45Z

@meltsufin @dmitry-s changes made from in-person discussion, PTAL

meltsufin · 2018-08-09T19:14:32Z

...in/java/org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerAutoConfiguration.java

@@ -86,6 +86,8 @@

 		private final int keepAliveIntervalMinutes;

+		private final boolean createTableDdlCascadeOnDelete;


This applies only to interleaved tables, correct?

Yes that's right

meltsufin · 2018-08-09T19:16:23Z

...in/java/org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerAutoConfiguration.java

@@ -105,6 +107,8 @@
 			this.writeSessionsFraction = gcpSpannerProperties.getWriteSessionsFraction();
 			this.keepAliveIntervalMinutes = gcpSpannerProperties
 					.getKeepAliveIntervalMinutes();
+			this.createTableDdlCascadeOnDelete = gcpSpannerProperties
+					.isCreateTableStatementsCascadeOnDelete();


inconsistency in naming createTableDdlCascadeOnDelete vs createTableStatementsCascadeOnDelete.

meltsufin · 2018-08-09T19:18:14Z

.../src/main/java/org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerProperties.java

@@ -40,6 +40,10 @@

 	private String database;

+	// if True then create-table statements generated will cascade on delete. no-action on delete


Please start sentences with an upper-case letter. Also, use {@code true}, and {@code false}.

technically (grammatically) these aren't sentences , but will change.

meltsufin · 2018-08-09T19:22:09Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

+	 * for generating DDL statements.
+	 * @param spannerEntityProcessor an entity processor that is queried for types that it
+	 * can convert for determining compatible column types when generating DDL statements.
+	 */
 	public SpannerSchemaUtils(SpannerMappingContext mappingContext,


I don't see a need for this constructor anymore. You always use the one below, except for one instance in test code.

Removed. Made requested changes.

@dmitry-s

meltsufin · 2018-08-09T19:42:40Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

@@ -47,76 +52,47 @@

 	private final SpannerTypeMapper spannerTypeMapper;

+	private final boolean createTableDdlDeleteOnCascade;


This is still different from createTableStatementsCascadeOnDelete. Also, I think a better name would actually indicate that this is for interleaved tables. Maybe createInterleavedTableCascadeOnDelete?

Yes I can change to that new name everywhere. (originally I felt it was OK to have "Ddl" in this class because so many things in this class have those letters in the name, and it would have more meaning given the context of this class and all the "Ddl" related functions.)

createTableStatementsCascadeOnDelete to use DDL in the name.
Actually, I'm leaning towards this name now:
createInterleavedTableDdlOnDeleteCascade

Renamed in all 3 places to createInterleavedTableDdlOnDeleteCascade and merged in other PRs and resolved conflicts.

I'll update the ref doc PR for this new name, too.

# Conflicts: # spring-cloud-gcp-data-spanner/src/main/java/org/springframework/cloud/gcp/data/spanner/core/mapping/SpannerPersistentEntity.java # spring-cloud-gcp-data-spanner/src/main/java/org/springframework/cloud/gcp/data/spanner/core/mapping/SpannerPersistentEntityImpl.java # spring-cloud-gcp-data-spanner/src/test/java/org/springframework/cloud/gcp/data/spanner/core/mapping/SpannerPersistentEntityImplTests.java

meltsufin · 2018-08-09T20:50:07Z

@ChengyuanZhao I think you need to resolve conflicts before merging.

stale, changes made

ChengyuanZhao added 5 commits July 25, 2018 13:40

Spanner schema generation for parent-child

3a5e444

Merge remote-tracking branch 'origin/master' into spanner-family-schema

1d2c9e6

# Conflicts: # spring-cloud-gcp-data-spanner/src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerSchemaUtils.java

Spanner schema admin functions and tests for parent-child

001d9c5

removed unnecessary check

74ba4b7

added property verification

e8ce757

ChengyuanZhao added the datastore GCP Datastore label Jul 28, 2018

ChengyuanZhao self-assigned this Jul 28, 2018

ChengyuanZhao requested review from dmitry-s and meltsufin July 28, 2018 02:50

ChengyuanZhao added spanner and removed datastore GCP Datastore labels Jul 28, 2018

meltsufin reviewed Jul 30, 2018

View reviewed changes

dmitry-s previously requested changes Jul 30, 2018

View reviewed changes

dmitry-s mentioned this pull request Jul 30, 2018

Spanner parent-child eager write save #911

Merged

pr comments

d5a2498

ChengyuanZhao commented Aug 2, 2018

View reviewed changes

meltsufin reviewed Aug 2, 2018

View reviewed changes

PR comments

6a393ca

rename

59b4487

meltsufin reviewed Aug 9, 2018

View reviewed changes

meltsufin suggested changes Aug 9, 2018

View reviewed changes

PR comments

6fa65de

meltsufin suggested changes Aug 9, 2018

View reviewed changes

rename to createInterleavedTableStatementCascadeOnDelete

e530232

meltsufin previously approved these changes Aug 9, 2018

View reviewed changes

Merging/resolving conflicts. rename setting

f542495

ChengyuanZhao dismissed meltsufin’s stale review via f542495 August 9, 2018 21:28

small refactoring

5bd9664

meltsufin approved these changes Aug 10, 2018

View reviewed changes

ChengyuanZhao merged commit c873e2b into master Aug 10, 2018

ChengyuanZhao deleted the spanner-family-schema branch August 10, 2018 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spanner schema admin functions for parent-child relationships #910

Spanner schema admin functions for parent-child relationships #910

ChengyuanZhao commented Jul 28, 2018

meltsufin Jul 30, 2018

ChengyuanZhao Jul 30, 2018 •

edited

dmitry-s Jul 30, 2018

ChengyuanZhao Aug 2, 2018

ChengyuanZhao left a comment

ChengyuanZhao Aug 2, 2018

ChengyuanZhao Aug 2, 2018

ChengyuanZhao Aug 2, 2018

ChengyuanZhao Aug 2, 2018

ChengyuanZhao Aug 2, 2018

meltsufin Aug 2, 2018

ChengyuanZhao commented Aug 2, 2018

meltsufin Aug 9, 2018

ChengyuanZhao Aug 9, 2018

meltsufin Aug 9, 2018

meltsufin Aug 9, 2018

ChengyuanZhao Aug 9, 2018

meltsufin Aug 9, 2018

ChengyuanZhao Aug 9, 2018

meltsufin Aug 9, 2018

ChengyuanZhao Aug 9, 2018

meltsufin Aug 9, 2018

ChengyuanZhao Aug 9, 2018

meltsufin commented Aug 9, 2018

		@@ -86,6 +86,8 @@

		private final int keepAliveIntervalMinutes;

		private final boolean createTableDdlCascadeOnDelete;

		@@ -40,6 +40,10 @@

		private String database;

		// if True then create-table statements generated will cascade on delete. no-action on delete

		@@ -47,76 +52,47 @@

		private final SpannerTypeMapper spannerTypeMapper;

		private final boolean createTableDdlDeleteOnCascade;

Spanner schema admin functions for parent-child relationships #910

Spanner schema admin functions for parent-child relationships #910

Conversation

ChengyuanZhao commented Jul 28, 2018

Choose a reason for hiding this comment

ChengyuanZhao Jul 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChengyuanZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChengyuanZhao commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin commented Aug 9, 2018

ChengyuanZhao Jul 30, 2018 •

edited