#2743 - Postgres DDL - For create table, improve column ordering for …

…tighter storage
ebean-orm · Jul 7, 2022 · 110bc61 · 110bc61
1 parent ec89d31
commit 110bc61
Show file tree

Hide file tree

Showing 10 changed files with 134 additions and 22 deletions.
diff --git a/...erator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PlatformDdl.java b/...erator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PlatformDdl.java
@@ -198,6 +198,7 @@ public String setLockTimeout(int lockTimeoutSeconds) {
    * Write all the table columns converting to platform types as necessary.
    */
   public void writeTableColumns(DdlBuffer apply, List<Column> columns, DdlIdentity identity) {
+    columns = sortColumns(columns);
     for (int i = 0; i < columns.size(); i++) {
       if (i > 0) {
         apply.append(",");
@@ -218,6 +219,11 @@ public void writeTableColumns(DdlBuffer apply, List<Column> columns, DdlIdentity
     }
   }
 
+  protected List<Column> sortColumns(List<Column> columns) {
+    // do nothing by default
+    return columns;
+  }
+
   /**
    * Write the column definition to the create table statement.
    */

diff --git a/...erator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdl.java b/...erator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdl.java
@@ -4,6 +4,14 @@
 import io.ebeaninternal.dbmigration.ddlgeneration.DdlBuffer;
 import io.ebeaninternal.dbmigration.ddlgeneration.DdlWrite;
 import io.ebeaninternal.dbmigration.migration.AlterColumn;
+import io.ebeaninternal.dbmigration.migration.Column;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toList;
 
 /**
  * Postgres specific DDL.
@@ -70,4 +78,52 @@ protected void alterColumnType(DdlWrite writer, AlterColumn alter) {
       .append(columnSetType).append(type)
       .append(" using ").append(alter.getColumnName()).append("::").append(type);
   }
+
+  @Override
+  protected List<Column> sortColumns(List<Column> columns) {
+    List<DDLColumnSort> sorting = new ArrayList<>(columns.size());
+    for (int i = 0, end = columns.size(); i < end; i++) {
+      Column column = columns.get(i);
+      sorting.add(new DDLColumnSort(column, ddlColumnOrdering(i, column)));
+    }
+    Collections.sort(sorting);
+    return sorting.stream().map(it -> it.column).collect(toList());
+  }
+
+  private int ddlColumnOrdering(int i, Column column) {
+    String type = column.getType().toLowerCase();
+    if (type.startsWith("decimal")) {
+      return i + 1_000;
+    }
+    if (isVariableLength(type) || isLob(type)) {
+      return i + 10_000;
+    }
+    return i;
+  }
+
+  private boolean isLob(String type) {
+    return type.startsWith("clob") || type.startsWith("longvarchar") || type.startsWith("blob") || type.startsWith("longvarbinary");
+  }
+
+  private boolean isVariableLength(String type) {
+    return type.startsWith("varchar") || type.startsWith("varbinary");
+  }
+
+  static final class DDLColumnSort implements Comparable<DDLColumnSort> {
+
+    private final Column column;
+    private final int ordering;
+
+    DDLColumnSort(Column column, int ordering) {
+      this.column = column;
+      this.ordering = ordering;
+    }
+
+    @Override
+    public int compareTo(DDLColumnSort o) {
+      return Integer.compare(ordering, o.ordering);
+    }
+  }
+
+
 }
diff --git a/...erator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/YugabyteDdl.java b/...erator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/YugabyteDdl.java
@@ -1,11 +1,19 @@
 package io.ebeaninternal.dbmigration.ddlgeneration.platform;
 
 import io.ebean.config.dbplatform.DatabasePlatform;
+import io.ebeaninternal.dbmigration.migration.Column;
 
-public class YugabyteDdl extends PostgresDdl {
+import java.util.List;
+
+public final class YugabyteDdl extends PostgresDdl {
 
   public YugabyteDdl(DatabasePlatform platform) {
     super(platform);
     this.historyDdl = new YugabyteHistoryDdl();
   }
+
+  @Override
+  protected List<Column> sortColumns(List<Column> columns) {
+    return columns;
+  }
 }
diff --git a/...or/src/test/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdlTest.java b/...or/src/test/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdlTest.java
@@ -1,18 +1,60 @@
 package io.ebeaninternal.dbmigration.ddlgeneration.platform;
 
 import io.ebean.platform.postgres.PostgresPlatform;
+import io.ebeaninternal.dbmigration.migration.Column;
 import org.junit.jupiter.api.Test;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import static org.assertj.core.api.Assertions.assertThat;
 
-public class PostgresDdlTest {
+class PostgresDdlTest {
 
   private PostgresDdl postgresDdl = new PostgresDdl(new PostgresPlatform());
 
   @Test
-  public void setLockTimeout() {
-
+  void setLockTimeout() {
     final String sql = postgresDdl.setLockTimeout(5);
     assertThat(sql).isEqualTo("set lock_timeout = 5000");
   }
+
+  @Test
+  void sortColumns_noAdjustment() {
+    List<Column> cols = postgresDdl.sortColumns(columns("integer", "bigint"));
+    assertThat(cols).extracting("type").containsExactly("integer", "bigint");
+
+    List<Column> colsReverse = postgresDdl.sortColumns(columns("bigint", "integer"));
+    assertThat(colsReverse).extracting("type").containsExactly("bigint", "integer");
+  }
+
+  @Test
+  void sortColumns_decimalVarchar() {
+    List<Column> cols = postgresDdl.sortColumns(columns("decimal(1)", "varchar(1)", "varchar(2)", "int", "decimal(2)"));
+    assertThat(cols).extracting("type").containsExactly("int", "decimal(1)", "decimal(2)", "varchar(1)", "varchar(2)");
+  }
+
+  @Test
+  void sortColumns_varbinary() {
+    List<Column> cols = postgresDdl.sortColumns(columns("decimal(1)", "varbinary(1)", "varbinary(2)", "int", "decimal(2)"));
+    assertThat(cols).extracting("type").containsExactly("int", "decimal(1)", "decimal(2)", "varbinary(1)", "varbinary(2)");
+  }
+
+  @Test
+  void sortColumns_clobs() {
+    List<Column> cols = postgresDdl.sortColumns(columns("clob", "blob", "longvarchar(1)", "int", "longvarbinary(2)"));
+    assertThat(cols).extracting("type").containsExactly("int", "clob", "blob", "longvarchar(1)", "longvarbinary(2)");
+  }
+
+  private List<Column> columns(String... types) {
+    int counter = 0;
+    List<Column> cols = new ArrayList<>();
+    for (String s : types) {
+      Column col = new Column();
+      col.setName("c" + counter++);
+      col.setType(s);
+      cols.add(col);
+    }
+    return cols;
+  }
 }
diff --git a/.../test/java/io/ebeaninternal/dbmigration/model/build/ModelBuild_explicitSequencesTest.java b/.../test/java/io/ebeaninternal/dbmigration/model/build/ModelBuild_explicitSequencesTest.java
@@ -16,7 +16,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-public class ModelBuild_explicitSequencesTest extends BaseTestCase {
+class ModelBuild_explicitSequencesTest extends BaseTestCase {
 
   private SpiEbeanServer createServer(boolean postgres) {
 
@@ -38,7 +38,7 @@ private SpiEbeanServer createServer(boolean postgres) {
   }
 
   @Test
-  public void test() throws IOException {
+  void test() throws IOException {
     SpiEbeanServer ebeanServer = createServer(false);
     try {
       CurrentModel currentModel = new CurrentModel(ebeanServer);
@@ -54,7 +54,7 @@ public void test() throws IOException {
   }
 
   @Test
-  public void test_asPostgres() throws IOException {
+  void test_asPostgres() throws IOException {
     SpiEbeanServer ebeanServer = createServer(true);
     try {
       CurrentModel currentModel = new CurrentModel(ebeanServer);

diff --git a/ebean-ddl-generator/src/test/resources/assert/ModelBuild_explicitSequencesTest/pg-apply.sql b/ebean-ddl-generator/src/test/resources/assert/ModelBuild_explicitSequencesTest/pg-apply.sql
@@ -7,8 +7,8 @@ create table PERSONS (
 
 create table PHONES (
   id                            bigint generated by default as identity not null,
-  phone_number                  varchar(7) not null,
   person_id                     bigint not null,
+  phone_number                  varchar(7) not null,
   constraint uq_phones_phone_number unique (phone_number),
   constraint pk_phones primary key (id)
 );

diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/1.0__initial.sql b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/1.0__initial.sql
@@ -14,9 +14,9 @@ create table migtest_ckey_detail (
 
 create table migtest_ckey_parent (
   one_key                       integer not null,
+  version                       integer not null,
   two_key                       varchar(127) not null,
   name                          varchar(255),
-  version                       integer not null,
   constraint pk_migtest_ckey_parent primary key (one_key,two_key)
 );
 
@@ -56,24 +56,24 @@ create table migtest_fk_set_null (
 
 create table migtest_e_basic (
   id                            integer generated by default as identity not null,
-  status                        varchar(1),
-  status2                       varchar(1) default 'N' not null,
-  name                          varchar(127),
-  description                   varchar(127),
   description_file              bytea,
   json_list                     json,
   a_lob                         varchar(255) default 'X' not null,
   some_date                     timestamptz,
   old_boolean                   boolean default false not null,
   old_boolean2                  boolean,
   eref_id                       integer,
+  user_id                       integer not null,
+  status                        varchar(1),
+  status2                       varchar(1) default 'N' not null,
+  name                          varchar(127),
+  description                   varchar(127),
   indextest1                    varchar(127),
   indextest2                    varchar(127),
   indextest3                    varchar(127),
   indextest4                    varchar(127),
   indextest5                    varchar(127),
   indextest6                    varchar(127),
-  user_id                       integer not null,
   constraint ck_migtest_e_basic_status check ( status in ('N','A','I')),
   constraint ck_migtest_e_basic_status2 check ( status2 in ('N','A','I')),
   constraint uq_migtest_e_basic_indextest2 unique (indextest2),

diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/idx_postgres.migrations b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/idx_postgres.migrations
@@ -1,4 +1,4 @@
-1014378814,     1.0__initial.sql
+2094204208,     1.0__initial.sql
 -2047324426,    1.1.sql
 -261111052,     1.2__dropsFor_1.1.sql
 1335621453,     1.3.sql

diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/1.0__initial.sql b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/1.0__initial.sql
@@ -14,9 +14,9 @@ create table migtest_ckey_detail (
 
 create table migtest_ckey_parent (
   one_key                       integer not null,
+  version                       integer not null,
   two_key                       varchar(127) not null,
   name                          varchar(255),
-  version                       integer not null,
   constraint pk_migtest_ckey_parent primary key (one_key,two_key)
 );
 
@@ -56,24 +56,24 @@ create table migtest_fk_set_null (
 
 create table migtest_e_basic (
   id                            serial not null,
-  status                        varchar(1),
-  status2                       varchar(1) default 'N' not null,
-  name                          varchar(127),
-  description                   varchar(127),
   description_file              bytea,
   json_list                     json,
   a_lob                         varchar(255) default 'X' not null,
   some_date                     timestamptz,
   old_boolean                   boolean default false not null,
   old_boolean2                  boolean,
   eref_id                       integer,
+  user_id                       integer not null,
+  status                        varchar(1),
+  status2                       varchar(1) default 'N' not null,
+  name                          varchar(127),
+  description                   varchar(127),
   indextest1                    varchar(127),
   indextest2                    varchar(127),
   indextest3                    varchar(127),
   indextest4                    varchar(127),
   indextest5                    varchar(127),
   indextest6                    varchar(127),
-  user_id                       integer not null,
   constraint ck_migtest_e_basic_status check ( status in ('N','A','I')),
   constraint ck_migtest_e_basic_status2 check ( status2 in ('N','A','I')),
   constraint uq_migtest_e_basic_indextest2 unique (indextest2),

diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/idx_postgres.migrations b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/idx_postgres.migrations
@@ -1,4 +1,4 @@
-382349675,      1.0__initial.sql
+-231429368,     1.0__initial.sql
 -606251140,     1.1.sql
 -261111052,     1.2__dropsFor_1.1.sql
 -893728811,     1.3.sql