Add Interleaved annotation implementation #168

dzou · 2019-11-21T17:20:15Z

This is the initial implementation of the @Interleaved annotation.

I would like to get this in and add the full polishing and integration tests in separate PRs.

In progress #140.

Notes:

You must control the order in which interleaved tables are created and dropped. When creating, parent tables must be created first before creating the interleaved tables. When dropping, interleaved tables must be dropped first before dropping the parent tables.
Sorting the PK columns in the create table statements for interleaved tables. The PK declaration of the child table must have a prefix which matches the PK declaration of the parent table. See SpannerTableStatements.getSortedPkColumns(..). Details: https://cloud.google.com/spanner/docs/schema-and-data-model

meltsufin

I think that the graph algorithm could potentially be separated into its own class and tested independently.
Just something to consider.
Also, please add more comments to the dense parts of the code.

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/Interleaved.java

...hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SpannerTableExporter.java

meltsufin · 2019-11-21T19:54:11Z

...ect/src/main/java/com/google/cloud/spanner/hibernate/schema/SpannerSchemaManagementTool.java

+  /**
+   * Returns a {@link Map} which maps a table to the table it is interleaved with.
+   */
+  private Map<Table, Table> buildCreateTableDependencies(Metadata metadata) {


Why does this method belong here rather than the SpannerTableExporter?

Done, I went with your suggestion of splitting out the table-dependency-tracking stuff to a separate class; this work is now done in that class.

meltsufin · 2019-11-21T19:54:33Z

...ect/src/main/java/com/google/cloud/spanner/hibernate/schema/SpannerSchemaManagementTool.java

+  /**
+   * Returns a {@link Map} which maps a table to the table that must be dropped before it.
+   */
+  private static Map<Table, Table> buildDropTableDependencies(Metadata metadata) {


Why does this method belong here rather than the SpannerTableExporter?
Also, please try to factor out the duplication with the method above.

Done; same as above.

...e-dialect/src/main/java/com/google/cloud/spanner/hibernate/schema/SpannerSchemaMigrator.java

meltsufin · 2019-11-21T19:59:21Z

...hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SpannerTableExporter.java


-  private final String createTableTemplate;
+  private Map<Table, Table> tableDependencies;


Which generic argument is parent and which is child?
I think the use of the term "dependent" or "dependencies" is a bit confusing.
Would it make sense to call the variables and methods more in terms of "child" tables and "parent" tables?

In the Map<Table, Table> for each (key, value) entry, the Table in the value must be processed (either created or dropped) before the Table in the key position.

meltsufin

I like the refactoring.

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/Interleaved.java

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SchemaUtils.java

...e-dialect/src/main/java/com/google/cloud/spanner/hibernate/schema/SpannerSchemaMigrator.java

dzou · 2019-11-21T23:17:15Z

Do we have something to test this?

Unfortunately right now we don't have test coverage over the SchemaMigrator. I think this is best tested through Integration test. I tried to write a unit test for this (i.e. running Hibernate in a unit test with ddl-mode.auto=UPDATE triggers the SchemaMigrator) but it tries to make queries to the underlying Spanner instance to see what the current tables are and see which tables need to be altered. I decided its not worth digging around in the Hibernate code and seeing what mock objects we need to inject in which fields to get this to work.

I created #170 to track the integration test efforts.

Note that create and drop are covered via unit tests.

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/Interleaved.java

...hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SpannerTableExporter.java

...bernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/TableDependencyTracker.java

…e/cloud/spanner/hibernate/Interleaved.java Co-Authored-By: Mike Eltsufin <meltsufin@google.com>

…loud-spanner-hibernate into interleaved

meltsufin

Some more, but minor comments.

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SchemaUtils.java

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/Interleaved.java

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SchemaUtils.java

...bernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SpannerTableStatements.java

...bernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/TableDependencyTracker.java

...e-dialect/src/main/java/com/google/cloud/spanner/hibernate/schema/SpannerSchemaMigrator.java

elefeint

Looks good, a few nits.

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/Interleaved.java

elefeint · 2019-12-02T16:32:24Z

...hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SpannerTableExporter.java

-     */
-
-    ArrayList<String> dropStrings = new ArrayList<>();
+  private String[] buildSqlStrings(Table currentTable, Metadata metadata, boolean isCreateTables) {


Non-blocking personal opinion: an enum would be clearer than boolean for CREATE vs DROP.

Agreed, but then you need to check for nulls and invalid options, etc.

I decided to use the Action Enum provided in the Hibernate libraries (it has Action.CREATE, Action.DROP, etc.)

It'll be fine to use this; as far as invalid options/nulls, I think we will be able to avoid these problems since these functions are not intended to be called by end users. It is more to just make the code easier for us to understand with enums over booleans.

...hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SpannerTableExporter.java

...r-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/schema/SchemaUtils.java

...-dialect/src/main/java/com/google/cloud/spanner/hibernate/schema/SpannerTableStatements.java

meltsufin

All good, after you address Elena's comments.

...-spanner-hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/Interleaved.java

meltsufin · 2019-12-02T18:44:25Z

...hibernate-dialect/src/main/java/com/google/cloud/spanner/hibernate/SpannerTableExporter.java

-     */
-
-    ArrayList<String> dropStrings = new ArrayList<>();
+  private String[] buildSqlStrings(Table currentTable, Metadata metadata, boolean isCreateTables) {


Agreed, but then you need to check for nulls and invalid options, etc.

Initial Interleaved implementation

de4d594

dzou requested review from dmitry-s, meltsufin and elefeint November 21, 2019 17:20

update doc

304e9a2

meltsufin requested changes Nov 21, 2019

View reviewed changes

dzou added 2 commits November 21, 2019 16:51

PR Comments

bb90917

Update Interleaved javadoc

3e82829

meltsufin requested changes Nov 21, 2019

View reviewed changes

dzou added 4 commits November 21, 2019 17:32

Add hibernate sequence insert to an init command.

f12af06

Fix test

a74b89c

rename parent to parentEntity

6e202d3

Simplify entity-to-table lookup

db0705f

dzou requested a review from meltsufin November 22, 2019 18:04

naming convention change and dropper

9d305f3

meltsufin requested changes Nov 25, 2019

View reviewed changes

dzou and others added 3 commits November 25, 2019 14:33

Update google-cloud-spanner-hibernate-dialect/src/main/java/com/googl…

1bde178

…e/cloud/spanner/hibernate/Interleaved.java Co-Authored-By: Mike Eltsufin <meltsufin@google.com>

PR Comments

fca0e76

Merge branch 'interleaved' of github.com:GoogleCloudPlatform/google-c…

ef2e580

…loud-spanner-hibernate into interleaved

dzou requested a review from meltsufin November 26, 2019 19:12

meltsufin requested changes Nov 27, 2019

View reviewed changes

dzou added 2 commits December 2, 2019 11:26

PR Comments

da59a05

fix integration tests

501508e

elefeint suggested changes Dec 2, 2019

View reviewed changes

meltsufin reviewed Dec 2, 2019

View reviewed changes

dzou added 3 commits December 2, 2019 13:56

PR Comments

43edd00

Remove void.class

bd79ce0

Fix checkstyle

2058836

meltsufin approved these changes Dec 2, 2019

View reviewed changes

dzou requested a review from elefeint December 2, 2019 19:14

elefeint approved these changes Dec 2, 2019

View reviewed changes

dzou merged commit 1733a4d into master Dec 2, 2019

dzou deleted the interleaved branch December 2, 2019 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Interleaved annotation implementation #168

Add Interleaved annotation implementation #168

dzou commented Nov 21, 2019

meltsufin left a comment

meltsufin Nov 21, 2019

dzou Nov 21, 2019

meltsufin Nov 21, 2019

dzou Nov 21, 2019

meltsufin Nov 21, 2019

dzou Nov 21, 2019

meltsufin left a comment

dzou commented Nov 21, 2019 •

edited

meltsufin left a comment

elefeint left a comment

elefeint Dec 2, 2019

meltsufin Dec 2, 2019

dzou Dec 2, 2019

meltsufin left a comment

meltsufin Dec 2, 2019


		private final String createTableTemplate;
		private Map<Table, Table> tableDependencies;

Add Interleaved annotation implementation #168

Add Interleaved annotation implementation #168

Conversation

dzou commented Nov 21, 2019

meltsufin 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

meltsufin left a comment

Choose a reason for hiding this comment

dzou commented Nov 21, 2019 • edited

meltsufin left a comment

Choose a reason for hiding this comment

elefeint 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

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzou commented Nov 21, 2019 •

edited