Merge lp:~tjoneslo/akiban-server/json-entity-name-collision-resolution into lp:~akiban-technologies/akiban-server/trunk

[Thomas Jones-Low]

Address potential name collision between fields and collections (Tables) in the JSON output. When the AISBuilder completes the process of building an AIS, the UserTable@endTable() is called. This in turn builds a unique name for the table in the group wrt the fields.

AISMerge#doAddIndexMerge() - Add call to builder#groupingIsComplete() to get the tables to generate their cached unique name.

UserTable@endTable() - generate the group unique name for the table (and child tables).

TableRowTracker - Update the tracker to use the UserTable unique name, rather than generating it on the fly.

AISBuilderTest - add tests for the UserTable name generate.

[Nathan Williams (nwilliams)]

Looks pretty good.

I think we can skip the "is same schema" logic. That was really working around the collapsed namespace issue, which this branch does explicitly.

Since they must be unused, can you delete the commented out members on UserTable?

review: Needs Fixing

[Thomas Jones-Low (tjoneslo)]

Remove dead code from UserTable.

review: Needs Resubmitting

[Nathan Williams (nwilliams)]

Thanks for the tweak!

Looks like the isSameSchema helper is commented out. Delete that and then feel free to big-A.

review: Needs Fixing

[Thomas Jones-Low (tjoneslo)]

Remove the newly dead code, before it becomes zombie code.

review: Approve

=== modified file 'src/main/java/com/akiban/ais/model/AISMerge.java'
--- src/main/java/com/akiban/ais/model/AISMerge.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/ais/model/AISMerge.java	2013-04-03 15:22:24 +0000
@@ -483,6 +483,7 @@
 
     private void doAddIndexMerge() {
         AISBuilder builder = new AISBuilder(targetAIS, nameGenerator);
+        builder.groupingIsComplete();
         builder.akibanInformationSchema().validate(AISValidations.LIVE_AIS_VALIDATIONS).throwIfNecessary();
         builder.akibanInformationSchema().freeze();
     }

=== modified file 'src/main/java/com/akiban/ais/model/UserTable.java'
--- src/main/java/com/akiban/ais/model/UserTable.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/ais/model/UserTable.java	2013-04-03 15:22:24 +0000
@@ -354,6 +354,44 @@
             assert primaryKeyIndex != null : this;
             primaryKey = new PrimaryKey(primaryKeyIndex);
         }
+        
+        // Put the columns into our list
+        TreeSet<String> entities = new TreeSet<String>();
+        for (Column column : getColumns()) {
+            entities.add(column.getName());
+        }
+
+        // put the child tables into their ordered list.
+        TreeMap<String, UserTable> childTables = new TreeMap<String, UserTable>();
+        for (Join childJoin : candidateChildJoins ) {
+            String childName;
+            if (childJoin.getChild().getName().getSchemaName().equals(getName().getSchemaName())) {
+                childName = childJoin.getChild().getName().getTableName();
+            } else {
+                childName = childJoin.getChild().getName().toString();
+            }
+            childTables.put(childName, childJoin.getChild());
+        }
+       
+        // Mangle the child table names to be unique with the "_"
+        for (String child : childTables.keySet()) {
+            String tryName = child;
+            while (entities.contains(tryName)) {
+                tryName = "_" + tryName;
+            }
+            childTables.get(child).nameForOutput = tryName;
+            entities.add(tryName);
+        }
+        
+        if (nameForOutput == null) {
+            Join parentJoin = getParentJoin();
+            if ((parentJoin != null) &&
+                    parentJoin.getParent().getName().getSchemaName().equals(getName().getSchemaName())) {
+                nameForOutput = getName().getTableName();
+            } else {
+                nameForOutput = getName().toString(); 
+            }
+        }
     }
 
     public Integer getDepth()
@@ -483,14 +521,8 @@
         this.version = version;
     }
 
-    public boolean isSchemaNameSameAsParent() {
-        Join parentJoin = getParentJoin();
-        return (parentJoin != null) &&
-                parentJoin.getParent().getName().getSchemaName().equals(getName().getSchemaName());
-    }
-
     public String getNameForOutput() {
-        return isSchemaNameSameAsParent() ? getName().getTableName() : getName().toString();
+        return nameForOutput;
     }
     
     private void addTableAndDescendents(UserTable table, List<UserTable> accumulator)
@@ -619,17 +651,16 @@
     private PrimaryKey primaryKey;
     private HKey hKey;
     private boolean containsOwnHKey;
-    private HKey branchHKey;
     private List<Column> allHKeyColumns;
     private Integer depth = null;
     private volatile List<UserTable> hKeyDependentTables;
-    private volatile List<UserTable> ancestors;
     private MemoryTableFactory tableFactory;
     private Integer version;
     private PendingOSC pendingOSC;
     private final Collection<FullTextIndex> fullTextIndexes;
     private final Collection<FullTextIndex> unmodifiableFullTextIndexes;
-
+    private String nameForOutput;
+    
     // consts
 
     private static final Comparator<Column> COLUMNS_BY_TABLE_DEPTH = new Comparator<Column>() {

=== modified file 'src/main/java/com/akiban/server/service/externaldata/TableRowTracker.java'
--- src/main/java/com/akiban/server/service/externaldata/TableRowTracker.java	2013-03-25 23:55:33 +0000
+++ src/main/java/com/akiban/server/service/externaldata/TableRowTracker.java	2013-04-03 15:22:24 +0000
@@ -22,9 +22,6 @@
 import com.akiban.qp.row.Row;
 import com.akiban.qp.rowtype.RowType;
 
-import java.util.HashSet;
-import java.util.Set;
-
 public class TableRowTracker implements RowTracker {
     private final int minDepth;
     private final int maxDepth;
@@ -34,13 +31,8 @@
     // rows, discarding any that are not.
     private final RowType[] openTypes;
 
-    // Tracks child tables where the schema name does not match the parent.
-    // Will almost always be empty, so use null as empty.
-    private Set<UserTable> tablesNeedingFullName = null;
-
     private RowType curRowType;
     private UserTable curTable;
-    private boolean curNeedsFullName;
 
     public TableRowTracker(UserTable table, int addlDepth) {
         minDepth = table.getDepth();
@@ -50,12 +42,6 @@
                 @Override
                 public void visitUserTable(UserTable userTable) {
                     max[0] = Math.max(max[0], userTable.getDepth());
-                    if(!userTable.isSchemaNameSameAsParent()) {
-                        if(tablesNeedingFullName == null) {
-                            tablesNeedingFullName = new HashSet<>();
-                        }
-                        tablesNeedingFullName.add(userTable);
-                    }
                 }
             });
         }
@@ -87,7 +73,6 @@
         assert row.rowType().hasUserTable() : "Invalid row type for TableRowTracker";
         curRowType = row.rowType();
         curTable = curRowType.userTable();
-        curNeedsFullName = (tablesNeedingFullName != null) && tablesNeedingFullName.contains(curTable);
     }
 
     @Override
@@ -97,7 +82,7 @@
 
     @Override
     public String getRowName() {
-        return curNeedsFullName ? curTable.getName().toString() : curTable.getName().getTableName();
+        return curTable.getNameForOutput();
     }
 
     @Override

=== modified file 'src/test/java/com/akiban/ais/model/AISBuilderTest.java'
--- src/test/java/com/akiban/ais/model/AISBuilderTest.java	2013-03-22 20:05:57 +0000
+++ src/test/java/com/akiban/ais/model/AISBuilderTest.java	2013-04-03 15:22:24 +0000
@@ -1235,4 +1235,96 @@
         AISValidationFailure fail = vResults.failures().iterator().next();
         Assert.assertEquals(ErrorCode.SEQUENCE_MIN_GE_MAX, fail.errorCode());
     }
+    
+    private AISBuilder twoChildGroup () {
+        final AISBuilder builder = new AISBuilder();
+        builder.userTable("test", "c");
+        builder.column("test", "c", "id", 0, "int", 0L, 0L, false, false, null, null);
+        builder.column("test", "c", "name", 1, "varchar", 64L, 0L, false, false, null, null);
+        builder.index("test", "c", Index.PRIMARY_KEY_CONSTRAINT, true, Index.PRIMARY_KEY_CONSTRAINT);
+        builder.indexColumn("test", "c", Index.PRIMARY_KEY_CONSTRAINT, "id", 0, true, null);
+        builder.userTable("test", "o");
+        builder.column("test", "o", "oid", 0, "int", 0L, 0L, false, false, null, null);
+        builder.column("test", "o", "cid", 1, "int", 0L, 0L, false, false, null, null);
+        builder.column("test", "o", "date", 2, "int", 0L, 0L, false, false, null, null);
+        builder.joinTables("c/id/o/cid", "test", "c", "test", "o");
+        builder.joinColumns("c/id/o/cid", "test", "c", "id", "test", "o", "cid");
+        builder.basicSchemaIsComplete();
+        TableName groupName = TableName.create("test", "coi");
+        builder.createGroup(groupName.getTableName(), groupName.getSchemaName());
+        builder.addJoinToGroup(groupName, "c/id/o/cid", 1);
+        builder.addTableToGroup(groupName, "test", "c");
+        builder.addTableToGroup(groupName, "test", "o");
+        
+        return builder;
+        
+    }
+    
+    @Test
+    public void validateNameForOutputSimple() {
+        AISBuilder builder = twoChildGroup();
+        
+        builder.groupingIsComplete();
+        UserTable c = builder.akibanInformationSchema().getUserTable("test", "c");
+        UserTable o = builder.akibanInformationSchema().getUserTable("test", "o");
+        
+        Assert.assertEquals("test.c", c.getNameForOutput());
+        Assert.assertEquals("o", o.getNameForOutput());
+    }
+    
+    @Test
+    public void validateNameForOutputCase1() {
+        AISBuilder builder = twoChildGroup();
+        builder.column("test", "c", "o", 2, "int", 0L, 0L, false, false, null, null);
+        builder.groupingIsComplete();
+        UserTable c = builder.akibanInformationSchema().getUserTable("test", "c");
+        UserTable o = builder.akibanInformationSchema().getUserTable("test", "o");
+        
+        Assert.assertEquals("test.c", c.getNameForOutput());
+        Assert.assertEquals("_o", o.getNameForOutput());
+    }
+    
+    @Test
+    public void validateNameForOutputCase2() {
+        AISBuilder builder = twoChildGroup();
+        builder.userTable("test", "_o");
+        builder.column("test", "_o", "oid", 0, "int", 0L, 0L, false, false, null, null);
+        builder.column("test", "_o", "cid", 1, "int", 0L, 0L, false, false, null, null);
+        builder.column("test", "_o", "date", 2, "int", 0L, 0L, false, false, null, null);
+        builder.joinTables("c/id/_o/cid", "test", "c", "test", "_o");
+        builder.joinColumns("c/id/_o/cid", "test", "c", "id", "test", "_o", "cid");
+        TableName groupName = TableName.create("test", "coi");
+        builder.addJoinToGroup(groupName, "c/id/_o/cid", 1);
+        builder.addTableToGroup(groupName, "test", "_o");
+        
+        builder.groupingIsComplete();
+        UserTable o = builder.akibanInformationSchema().getUserTable("test", "o");
+        UserTable _o = builder.akibanInformationSchema().getUserTable("test", "_o");
+        
+        Assert.assertEquals("o", o.getNameForOutput());
+        Assert.assertEquals("_o", _o.getNameForOutput());
+    }
+    
+    @Test
+    public void validateNameForOutputCase3() {
+        AISBuilder builder = twoChildGroup();
+        builder.column("test", "c", "o", 2, "int", 0L, 0L, false, false, null, null);
+        builder.userTable("test", "_o");
+        builder.column("test", "_o", "oid", 0, "int", 0L, 0L, false, false, null, null);
+        builder.column("test", "_o", "cid", 1, "int", 0L, 0L, false, false, null, null);
+        builder.column("test", "_o", "date", 2, "int", 0L, 0L, false, false, null, null);
+        builder.joinTables("c/id/_o/cid", "test", "c", "test", "_o");
+        builder.joinColumns("c/id/_o/cid", "test", "c", "id", "test", "_o", "cid");
+        TableName groupName = TableName.create("test", "coi");
+        builder.addJoinToGroup(groupName, "c/id/_o/cid", 1);
+        builder.addTableToGroup(groupName, "test", "_o");
+        
+        builder.groupingIsComplete();
+        UserTable o = builder.akibanInformationSchema().getUserTable("test", "o");
+        UserTable _o = builder.akibanInformationSchema().getUserTable("test", "_o");
+        
+        Assert.assertEquals("__o", o.getNameForOutput());
+        Assert.assertEquals("_o", _o.getNameForOutput());
+    }
+    
 }