You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by twdsilva <gi...@git.apache.org> on 2018/08/31 01:22:42 UTC
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
GitHub user twdsilva opened a pull request:
https://github.com/apache/phoenix/pull/339
PHOENIX-4855 Continue to write base table column metadata when creati…
…ng a view in order to support rollback
Previously in MetadataEndpointImpl we were removing parent column metadata while creating a child view. We still need to write the parent columns in order to be able to rollback if needed. This PR filters out the parent columns while creating the PTable of a child view. When a view is resolved we also resolve all its parents and add their columns (see MetadataEndpointImpl.addDerivedColumnsFromAncestors).
I added some more tests for salted tables. I also changed PhoenixDatabaseMetaData.getPrimaryKeys() to load the PTable and return the metadata instead of directly querying the SYSTEM.CATALOG table (which will not work any more since we don't store columns inherited from parents in a view)
@ankitsinghal @ChinmaySKulkarni can you please review?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/twdsilva/phoenix PHOENIX-4855
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/339.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #339
----
commit a352c6ec72204cda7c09556e296139b7d5cc92dd
Author: Thomas D'Silva <td...@...>
Date: 2018-08-20T17:42:56Z
PHOENIX-4855 Continue to write base table column metadata when creating a view in order to support rollback
----
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/339#discussion_r222153120
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -3243,20 +3270,31 @@ public MetaDataMutationResult updateMutation(PTable table, byte[][] rowKeyMetaDa
addingCol = true;
if (pkCount > FAMILY_NAME_INDEX
&& rowKeyMetaData[PhoenixDatabaseMetaData.FAMILY_NAME_INDEX].length > 0) {
+ byte[] familyName = rowKeyMetaData[PhoenixDatabaseMetaData.FAMILY_NAME_INDEX];
+ byte[] columnName = rowKeyMetaData[PhoenixDatabaseMetaData.COLUMN_NAME_INDEX];
+ if (table.getExcludedColumns().contains(
+ PColumnImpl.createExcludedColumn(newPName(familyName), newPName(columnName), 0l))) {
+ // if this column was previously dropped in a view do not allow adding the column back
+ return new MetaDataMutationResult(
--- End diff --
Only disallow adding it back if the data type is different instead?
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/339#discussion_r222145718
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterTableWithViewsIT.java ---
@@ -810,59 +827,65 @@ public void testDivergedViewsStayDiverged() throws Exception {
PTable table = PhoenixRuntime.getTableNoCache(viewConn, view1);
assertEquals(QueryConstants.DIVERGED_VIEW_BASE_COLUMN_COUNT, table.getBaseColumnCount());
+ try {
+ viewConn.createStatement().execute("SELECT V2 FROM " + view1);
+ fail("V2 should have been droppped");
+ } catch (SQLException e) {
+ assertEquals(SQLExceptionCode.COLUMN_NOT_FOUND.getErrorCode(), e.getErrorCode());
+ }
+
// Add a new regular column and pk column to the base table
String alterBaseTable = "ALTER TABLE " + baseTable + " ADD V3 VARCHAR, PK2 VARCHAR PRIMARY KEY";
conn.createStatement().execute(alterBaseTable);
// Column V3 shouldn't have propagated to the diverged view.
- String sql = "SELECT V3 FROM " + view1;
try {
- viewConn.createStatement().execute(sql);
+ viewConn.createStatement().execute("SELECT V3 FROM " + view1);
--- End diff --
Assert.fail in case this exception is not caught. Similarly for all other places where we expect an exception.
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva closed the pull request at:
https://github.com/apache/phoenix/pull/339
---
[GitHub] phoenix issue #339: PHOENIX-4855 Continue to write base table column metadat...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on the issue:
https://github.com/apache/phoenix/pull/339
A few comments. Please also address the test failures inside `ViewIT.java`. Otherwise, lgtm.
---
[GitHub] phoenix issue #339: PHOENIX-4855 Continue to write base table column metadat...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/339
ping @ChinmaySKulkarni @karanmehta93 any chance I can get this reviewed soon?
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/339#discussion_r218645260
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -1879,42 +1932,6 @@ public void createTable(RpcController controller, CreateTableRequest request,
dropChildMetadata(schemaName, tableName, tenantIdBytes);
}
- // Here we are passed the parent's columns to add to a view, PHOENIX-3534 allows for a splittable
--- End diff --
You can mark PHOENIX-4767 fixed as well, with this PR.
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/339#discussion_r222150790
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -718,9 +731,9 @@ private PTable addDerivedColumnsFromAncestors(PTable table, long timestamp,
List<PColumn> excludedColumns = Lists.newArrayList();
--- End diff --
These are for primary key columns present in the base table, but dropped in the view, right? Let's rename this to `excludedPkColumns`
---
[GitHub] phoenix issue #339: PHOENIX-4855 Continue to write base table column metadat...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on the issue:
https://github.com/apache/phoenix/pull/339
@twdsilva Had a quick glance over the PR. I had a few questions. So in the case a view is created on top of a salted base table, we explicitly filter out the salting column (first column) when resolving the columns for a view, right? Also, in case any ancestor of a view drops a column, that will be part of the excluded columns list and we filter out those columns as well, when resolving a view? What happens if a view diverges from the base table, the base table drops a column and then the diverged view re-adds this column?
Overall, can you please add more comments to make it easier to understand the steps taken when handling excluded columns? I will go over the PR in detail and let you know some more feedback. Thanks
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/339#discussion_r222475810
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterTableWithViewsIT.java ---
@@ -810,59 +827,65 @@ public void testDivergedViewsStayDiverged() throws Exception {
PTable table = PhoenixRuntime.getTableNoCache(viewConn, view1);
assertEquals(QueryConstants.DIVERGED_VIEW_BASE_COLUMN_COUNT, table.getBaseColumnCount());
+ try {
+ viewConn.createStatement().execute("SELECT V2 FROM " + view1);
+ fail("V2 should have been droppped");
+ } catch (SQLException e) {
+ assertEquals(SQLExceptionCode.COLUMN_NOT_FOUND.getErrorCode(), e.getErrorCode());
+ }
+
// Add a new regular column and pk column to the base table
String alterBaseTable = "ALTER TABLE " + baseTable + " ADD V3 VARCHAR, PK2 VARCHAR PRIMARY KEY";
conn.createStatement().execute(alterBaseTable);
// Column V3 shouldn't have propagated to the diverged view.
- String sql = "SELECT V3 FROM " + view1;
try {
- viewConn.createStatement().execute(sql);
+ viewConn.createStatement().execute("SELECT V3 FROM " + view1);
--- End diff --
Done
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/339#discussion_r218630691
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java ---
@@ -226,10 +228,13 @@ public PTableImpl(PName tenantId, PName schemaName, PName tableName, long timest
familyByString.put(family.getName().getString(), family);
}
this.families = families;
+ if (bucketNum!=null) {
--- End diff --
Can you add a comment as to why we are leaving out the first column here?
---
[GitHub] phoenix pull request #339: PHOENIX-4855 Continue to write base table column ...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/339#discussion_r222150196
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -797,7 +810,7 @@ private PTable addDerivedColumnsFromAncestors(PTable table, long timestamp,
if (hasIndexId) {
// add all pk columns of parent tables to indexes
// skip salted column as it will be added from the base table columns
- startIndex = pTable.getBucketNum() != null ? 1 : 0;
+ int startIndex = pTable.getBucketNum() != null ? 1 : 0;
for (int index=startIndex; index<pTable.getPKColumns().size(); index++) {
PColumn column = pTable.getPKColumns().get(index);
--- End diff --
Please rename `column` to `pkColumn` to make it easier to understand
---