Support table creation DDL translation #194

francisco-polaco · 2021-01-05T17:54:59Z

Closes #191

src/main/java/com/feedzai/commons/sql/abstraction/engine/AbstractTranslator.java

Co-authored-by: João Fernandes <[email protected]>

jmf-tls

Some things are not working properly and need to be changed...

Also, it would be nice to have a test for the translateTableCreation, something like:

create a test entity (but don't add it to the DB with engine.addEntity)
translate the entity
split the resulting string into the different statements to execute
execute all the statements to create the table, sidelining PDB (should not throw any exceptions)
load entity with engine.loadEntity
(PDB) persist something in the entity and select all to make sure the entry was really persisted

jmf-tls · 2021-01-18T14:29:09Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/DatabaseEngine.java

+     * @param entity The entity to translate.
+     * @return The translation result.
+     */
+    String translateTableCreation(final DbEntity entity);


This is a breaking change that will require classes that implement this interface
(e.g. wrappers for instrumentation) to implement the new method, otherwise compilation fails.

Let's avoid this, by turning this method into a default method that throws an UnsupportedOperationException or something like that.

jmf-tls · 2021-01-18T14:39:28Z

src/main/java/com/feedzai/commons/sql/abstraction/ddl/DbEntity.java

+        return translator.translateCreateTable(this)
+                + translator.translatePrimaryKeysNotNull(this)
+                + translator.translatePrimaryKeysConstraints(this)
+                + translator.translateForeignKey(this)
+                + translator.translateCreateIndexes(this)
+                + translator.translateCreateSequences(this);


I just checked this with H2 and PostgreSQL on the following entity:

dbEntity() .name("TEST") .addColumn("PK_COL", INT, true) .addColumn("COL1", INT, new K(1)) .addColumn("COL2", BOOLEAN, new K(false), NOT_NULL) .addColumn("COL3", DOUBLE, new K(2.2d)) .addColumn("COL4", LONG, new K(3L)) .pkFields("PK_COL") .addIndex(true, "COL1") .build();

With H2 it doesn't work, throws UnsupportedOperationException (see my comment in H2Translator.java);
with PostgreSQL, the result is

CREATE TABLE "TEST" ("PK_COL" SERIAL, "COL1" INT DEFAULT 1, "COL2" BOOLEAN NOT NULL DEFAULT FALSE, "COL3" DOUBLE PRECISION DEFAULT 2.2, "COL4" BIGINT DEFAULT 3)ALTER TABLE "TEST" ADD CONSTRAINT "a8175238f007307f7bdfb531ee9943" PRIMARY KEY ("PK_COL")[][CREATE UNIQUE INDEX "bb260359a58a48fbfcb93859e498b7" ON "TEST" ("COL1")][]

This is kind of messy to look at in a log, and it is almost useless as input for another tool.

each translated statement (CREATE TABLE, CREATE INDEX, etc) should end with ; and start in a new line.

some of the translations are strings, some are lists, which results in those []; the translations should all result in strings.
Maybe you could use something like

Stream.of(translator.translateCreateTable(this), ......) .filter(stmt -> !stmt.isEmpty()) .collect(Collectors.joining(";\n"));

(each translate* that returns a list should first be converted into a string with String.join(";\n", list))
By following that, we get

CREATE TABLE "TEST" ("PK_COL" SERIAL, "COL1" INT DEFAULT 1, "COL2" BOOLEAN NOT NULL DEFAULT FALSE, "COL3" DOUBLE PRECISION DEFAULT 2.2, "COL4" BIGINT DEFAULT 3); ALTER TABLE "TEST" ADD CONSTRAINT "a8175238f007307f7bdfb531ee9943" PRIMARY KEY ("PK_COL"); CREATE UNIQUE INDEX "bb260359a58a48fbfcb93859e498b7" ON "TEST" ("COL1")

jmf-tls · 2021-01-18T15:20:56Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/AbstractTranslator.java

+     * @param entity The entity to translate.
+     * @return The foreign key constraints translation result.
+     */
+    public abstract List<String> translateForeignKey(DbEntity entity);


translateForeignKeys
(plural)

jmf-tls · 2021-01-18T16:04:57Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/AbstractTranslator.java

+    /**
+     * Translates the primary key not null constraints of the given entity table to the current dialect.
+     *
+     * @param entity The entity to translate.
+     * @return The primary key not null constraints translation result.
+     */
+    public String translatePrimaryKeysNotNull(DbEntity entity) {
+        // usually engines don't need to specify columns as not nulls to be PK.
+        return "";
+    }


When creating a table, the PRIMARY KEY columns should already be specified as NOT NULL if really needed.
H2 is doing it, and so should do DB2 and SQL Server; then this method won't be needed.

I suppose that the other engines that don't need this already assume that the PRIMARY KEY column is NOT NULL implicitly.

jmf-tls · 2021-01-18T16:05:52Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/AbstractTranslator.java

+     * @param entity The entity to translate.
+     * @return The primary key constraints translation result.
+     */
+    public abstract String translatePrimaryKeysConstraints(DbEntity entity);


if the method translatePrimaryKeysNotNullis removed as suggested,
this one can be just translatePrimaryKeys

jmf-tls · 2021-01-19T10:54:22Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/impl/H2Translator.java

+                // Create a NOT NULL constraint
+                c.getColumnConstraints().add(DbColumnConstraint.NOT_NULL);


This causes problems, because the list of column constraints is immutable.
It didn't break before inside the "createTable" because the method "injectNotNullIfMissing" was already adding the NOT NULLs where needed.
But we don't need any of that, we can add the constraint directly to the column list:

Suggested change

// Create a NOT NULL constraint

c.getColumnConstraints().add(DbColumnConstraint.NOT_NULL);

column.add(DbColumnConstraint.NOT_NULL.translate());

jmf-tls · 2021-01-19T11:51:05Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/impl/PostgreSqlEngine.java

            return;
        }


For this and other engines/translators, this verification

if (entity.getPkFields().size() == 0) { return; }

shoud be in the translator.
Otherwise, in the case of PostgreSQL we get something like
ALTER TABLE "table_name" ADD CONSTRAINT "812t6737613t7edqw" PRIMARY KEY ()
(alter table to add primary key with no columns) which doesn't make sense...

jmf-tls · 2021-01-19T12:11:53Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/impl/CockroachDBTranslator.java

+    }
+
+    @Override
+    public List<String> translateCreateIndexes(final DbEntity entity) {


We should take a look at the tests, they didn't fail with this change and they should...
You correctly copied the code from the CockroachDBEngine, but from addSequences:
this method should be called translateCreateSequences;
the addIndexes is inherited from PostgreSQL engine, and thus the translateCreateIndexes should also be inherited from PostgreSQL translator.

jmf-tls · 2021-01-19T12:18:34Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/impl/DB2Translator.java

+            final String alterTable = format("ALTER TABLE %s ADD CONSTRAINT %s FOREIGN KEY (%s) REFERENCES %s (%s)",
+                                             table,
+                                             quotize(md5(
+                                                     "FK_" + table + quotizedLocalColumnsSting + quotizedForeignColumnsString,
+                                                     properties.getMaxIdentifierSize()
+                                             )),
+                                             quotizedLocalColumnsSting,
+                                             quotize(fk.getForeignTable()),
+                                             quotizedForeignColumnsString
+            );


strange indentation... so much to the right that you had to break the "quotize(md5" line into 4 lines

jmf-tls · 2021-01-19T12:24:52Z

src/main/java/com/feedzai/commons/sql/abstraction/engine/impl/OracleEngine.java

    private String getCreateTableStatement(final DbEntity entity) throws DatabaseEngineException {
-        final List<String> createTable = new ArrayList<>();
-
-        createTable.add("CREATE TABLE");
-        createTable.add(quotize(entity.getName()));
-        final List<String> columns = new ArrayList<>();
-        for (final DbColumn c : entity.getColumns()) {
-            final List<String> column = new ArrayList<>();
-            column.add(quotize(c.getName()));
-            column.add(translateType(c));
-
-            if (c.isDefaultValueSet() && !c.getDbColumnType().equals(DbColumnType.BOOLEAN)) {
-                column.add("DEFAULT");
-                column.add(translate(c.getDefaultValue()));
-            }
-
-            for (final DbColumnConstraint cc : c.getColumnConstraints()) {
-                column.add(cc.translate());
-            }
-            columns.add(join(column, " "));
-        }
-        createTable.add("(" + join(columns, ", ") + ")");
-        // segment creation deferred can cause unexpected behaviour in sequences
-        // see: http://dirknachbar.blogspot.pt/2011/01/deferred-segment-creation-under-oracle.html
-        createTable.add("SEGMENT CREATION IMMEDIATE");
-        return join(createTable, " ");
+        return translator.translateCreateTable(entity);
    }


This method is not needed anymore...
In the createTable method above, you can simply use the translator method directly:

final String defaultCreateTableStatement = translator.translateCreateTable(entity);

Also, on the createTablemethod, right now we have this:

defaultCreateTableStatement = get createTable from translator createTableCompressLOBS = getCompressLobsStatements(entity); createTableStatement = defaultCreateTableStatement + createTableCompressLOBS;

Then we try to create the table with createTableStatement, which may or may not contain LOB options depending on PDB config to compress LOBs (if the option is disabled, this statement is the same as defaultCreateTableStatement).
If this create table fails because the DB doesn't support LOBs, then we have a retry with the default statement (without LOBs).

I would propose a change to this logic: have a single statement to create tables, with or without LOB options according to the config. This statement would be produced at translator.translateCreateTable(entity) as it is now, but would include the LOB options (if applicable).
This would make the logic much simpler, the method getCompressLobsStatements would go away and its code integrated in the translateCreateTable.

As it is now, with "compress LOBs" enabled PDB will either create the table with LOB compression, or if the DB doesn't support it, will create the table without compression.... with the proposed change, in the same scenario PDB would will either create the table with LOB compression or fail.
However, I believe that this is acceptable: it is up to whoever sets up the environment to make sure that the DB supports LOB compression, or if it doesn't, configure PDB with LOB compression disabled.
What do you think?
Also, @jcsf @joaonuno

Francisco Santos added 6 commits January 4, 2021 19:39

ref PDB-191: Create table translation refactor

bccac22

ref PDB-191: Private keys constraints refactor

838517f

ref PDB-191: Foreign keys refactor

d629a16

ref PDB-191: Add index refactor

31ab485

ref PDB-191: Add sequence refactor

1c0e801

ref PDB-191: Documentation

2bc24a3

francisco-polaco requested review from joaonuno and jmf-tls January 5, 2021 17:54

francisco-polaco self-assigned this Jan 5, 2021

Francisco Santos added 2 commits January 5, 2021 17:57

ref PDB-191: Documentation

7d9bb8d

ref PDB-191: Fix imports

81fd866

jcsf reviewed Jan 11, 2021

View reviewed changes

src/main/java/com/feedzai/commons/sql/abstraction/engine/AbstractTranslator.java Outdated Show resolved Hide resolved

src/main/java/com/feedzai/commons/sql/abstraction/engine/AbstractTranslator.java Outdated Show resolved Hide resolved

Apply suggestions from code review

80ad018

Co-authored-by: João Fernandes <[email protected]>

jcsf approved these changes Jan 15, 2021

View reviewed changes

jmf-tls requested changes Jan 19, 2021

View reviewed changes

TiagoMSSantos closed this Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support table creation DDL translation #194

Support table creation DDL translation #194

francisco-polaco commented Jan 5, 2021 •

edited

Loading

jmf-tls left a comment •

edited

Loading

jmf-tls Jan 18, 2021

jmf-tls Jan 18, 2021

jmf-tls Jan 18, 2021

jmf-tls Jan 18, 2021

jmf-tls Jan 18, 2021

jmf-tls Jan 19, 2021

jmf-tls Jan 19, 2021

jmf-tls Jan 19, 2021

jmf-tls Jan 19, 2021

jmf-tls Jan 19, 2021

jmf-tls Jan 19, 2021

		// Create a NOT NULL constraint
		c.getColumnConstraints().add(DbColumnConstraint.NOT_NULL);

	// Create a NOT NULL constraint
	c.getColumnConstraints().add(DbColumnConstraint.NOT_NULL);
	column.add(DbColumnConstraint.NOT_NULL.translate());

Support table creation DDL translation #194

Support table creation DDL translation #194

Conversation

francisco-polaco commented Jan 5, 2021 • edited Loading

jmf-tls left a comment • edited Loading

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

Choose a reason for hiding this comment

francisco-polaco commented Jan 5, 2021 •

edited

Loading

jmf-tls left a comment •

edited

Loading