Phoenix 3534 #248
Phoenix 3534 #248
Conversation
… additive case where we take lower timestamp
…ombine logic for excluded columns
|
I'll check it out, but in the meantime would you mind amending your commit message from "Phoenix 3534" to "PHOENIX-3534 Support multi region SYSTEM.CATALOG table" - that way comments on the pull will become JIRA comments. |
|
@twdsilva would you mind taking a look as well? |
| Collections.sort(sortedColumns, new Comparator<PColumn>() { | ||
| @Override | ||
| public int compare(PColumn o1, PColumn o2) { | ||
| return Integer.valueOf(o1.getPosition()).compareTo(o2.getPosition()); |
There was a problem hiding this comment.
What will prevent overlapping ordinals between a base table and derived views?
| @@ -107,12 +107,14 @@ public void testAddNewColumnsToBaseTableWithViews() throws Exception { | |||
| assertTableDefinition(conn, tableName, PTableType.TABLE, null, 0, 3, QueryConstants.BASE_TABLE_BASE_COLUMN_COUNT, "ID", "COL1", "COL2"); | |||
|
|
|||
| viewConn.createStatement().execute("CREATE VIEW " + viewOfTable + " ( VIEW_COL1 DECIMAL(10,2), VIEW_COL2 VARCHAR ) AS SELECT * FROM " + tableName); | |||
| assertTableDefinition(conn, viewOfTable, PTableType.VIEW, tableName, 0, 5, 3, "ID", "COL1", "COL2", "VIEW_COL1", "VIEW_COL2"); | |||
| assertTableDefinition(conn, viewOfTable, PTableType.VIEW, tableName, 0, 5, 3, "VIEW_COL1", "VIEW_COL2"); | |||
There was a problem hiding this comment.
Are these test changes due to only storing diffs now? Would be nice to keep the check for "this is what the combined table should look like" in addition to checking that only the diffs are actually stored. WDYT?
There was a problem hiding this comment.
Sure we can do that, I think we will need to modify a ton of tests to make this happen. Its not only done here.
Another thing is that there are a few tests where the assumptions of how the system works has changed. A few examples are tests like this:
ViewIT.testDisallowDropOfColumnOnParentTable()
ViewIT.testDisallowDropOfReferencedColumn()
QueryCompiler.testDuplicatePKColumn()
(this one because we have to support both the diff based approach and the current approach for our migration step, we remove duplicate columns on read, thus we don't prevent this from happening).
There are quite a few more, we can maybe discuss if these tests are no longer valid or how we can change them to make them meaningful.
| } | ||
| // else { |
There was a problem hiding this comment.
You'll likely get to this later, but let's remove the commented out code when you're ready.
| @@ -159,15 +199,6 @@ public SortOrder getSortOrder() { | |||
| public String toString() { | |||
| return (familyName == null ? "" : familyName.toString() + QueryConstants.NAME_SEPARATOR) + name.toString(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Should this be removed and if so why?
| @@ -381,7 +383,8 @@ | |||
| IS_VIEW_REFERENCED_KV, | |||
| COLUMN_DEF_KV, | |||
| IS_ROW_TIMESTAMP_KV, | |||
| COLUMN_QUALIFIER_KV | |||
| COLUMN_QUALIFIER_KV, | |||
| DROPPED_COLUMN_KV | |||
There was a problem hiding this comment.
I don't think you need this DROPPED_COLUMN_KV since you're not reading this when the column is read (but instead when the linking rows are read). If that's right, let's remove it.
|
This is looking really good, @Churrodog. For testing purposes, I think it'd be really good if we split the SYSTEM.CATALOG table at points that'll force it to span multiple regions during the running of tests like AlterTableWithViewsIT, TenantSpecificTablesDDLIT, AlterTableIT, QueryDatabaseMetaDataIT, and maybe more. Any full table scans let in the CREATE or DROP code path? |
| /** | ||
| * Certain operations, such as DROP TABLE are not allowed if there a table has child views. This class wraps the | ||
| * Results of a scanning the Phoenix Metadata for child views for a specific table and stores an additional flag for | ||
| * whether whether SYSTEM.CATALOG has split across multiple regions. |
| byte[] tableInQuestion = listOBytes.get(i); | ||
| PTable pTable = this.doGetTable(tableInQuestion, timestamp); | ||
| if (pTable == null) { | ||
| throw new TableNotFoundException("fill in with valuable info"); |
There was a problem hiding this comment.
fix the exception message
| } | ||
| } | ||
|
|
||
| static Graph<TableInfo> findOrphans(Table systemCatalog, long timestamp) throws IOException { |
There was a problem hiding this comment.
is this method used somewhere?
…t always seem to work
Conflicts: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/WhereConstantParser.java phoenix-core/src/test/java/org/apache/phoenix/coprocessor/MetaDataEndpointImplTest.java
|
Looks like there are still conflicts, @Churrodog. Would you mind amending your commit message to "PHOENIX-3534 Support multi region SYSTEM.CATALOG table" so that the JIRA is linked to the PR? |
|
Already merged. |
I'll squash these down after a review