Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support table creation DDL translation #194

Closed
wants to merge 9 commits into from

Conversation

francisco-polaco
Copy link
Contributor

@francisco-polaco francisco-polaco commented Jan 5, 2021

Closes #191

Co-authored-by: João Fernandes <jcsf_1995@hotmail.com>
Copy link
Member

@jmf-tls jmf-tls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +153 to +158
return translator.translateCreateTable(this)
+ translator.translatePrimaryKeysNotNull(this)
+ translator.translatePrimaryKeysConstraints(this)
+ translator.translateForeignKey(this)
+ translator.translateCreateIndexes(this)
+ translator.translateCreateSequences(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

translateForeignKeys
(plural)

Comment on lines +645 to +654
/**
* 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 "";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +372 to +373
// Create a NOT NULL constraint
c.getColumnConstraints().add(DbColumnConstraint.NOT_NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Comment on lines 257 to 258
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

}

@Override
public List<String> translateCreateIndexes(final DbEntity entity) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +498 to +507
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
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 459 to 461
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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support translating table creation DDL
4 participants