phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Taylor (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (PHOENIX-1504) Support adding column to a table that has views
Date Wed, 17 Jun 2015 20:06:00 GMT

    [ https://issues.apache.org/jira/browse/PHOENIX-1504?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14590475#comment-14590475
] 

James Taylor commented on PHOENIX-1504:
---------------------------------------

Great work, [~samarthjain]. Excellent tests. Couple of minor nits to fix on commit:
- ViewIT.java looks like it has not substantive changes, so please revert.
- Any chance that BASE_COLUMN_COUNT will be null (i.e. pre-commit of the upgrade script) here
in MetaDataEndPointImpl? Maybe a check for null with a default value would be prudent here?
{code}
+        Cell baseColumnCountKv = tableKeyValues[BASE_COLUMN_COUNT_INDEX];
+        Integer baseColumnCount = PInteger.INSTANCE.getCodec().decodeInt(baseColumnCountKv.getValueArray(),
+            baseColumnCountKv.getValueOffset(), SortOrder.getDefault());
{code}
- Not sure if I'm seeing double here post Warriror's championship party, but seems that this
is repeated?
{code}
+        scan.addColumn(TABLE_FAMILY_BYTES, TABLE_SEQ_NUM_BYTES);
+
+        scan.addColumn(TABLE_FAMILY_BYTES, TABLE_SEQ_NUM_BYTES);
{code}
- Rather than pass a boolean isAdd into mutateColumn(), how about removing the findChildViews
logic and having it be different in the updateMutation implementation provided by dropColumn
and addColumn? In dropColumn, you'd just return a MutationCode.UNALLOWED_TABLE_MUTATION if
there are child views. In addColumn, you'd return that if the metadata spans multiple regions
or if a view has child views.
{code}
+                    TableViewFinderResult childViewsResult =
+                            findChildViews(region, tenantId, table,
+                                (type == PTableType.VIEW ? PARENT_TABLE_BYTES
+                                        : PHYSICAL_TABLE_BYTES));
+                    if (childViewsResult.hasViews()) {
+                        /*
+                         * Table mutations are not allowed if:
+                         * 1) Metadata for child views is split over more than one region.
+                         * 2) Dropping columns for tables or views that have child views
+                         * 3) Adding columns for views that have child views.
+                         * 
+                         */
+                        if (!childViewsResult.allViewsInSingleRegion() || !isAdd || (type
== PTableType.VIEW && isAdd)) {
+                            return new MetaDataMutationResult(
+                                    MutationCode.UNALLOWED_TABLE_MUTATION,
+                                    EnvironmentEdgeManager.currentTimeMillis(), null);
+                        } else {
+                            addRowsToChildViews(tableMetadata, schemaName, tableName,
+                                viewMutations, invalidateList, clientTimeStamp, childViewsResult,
+                                region, locks);
+                        }
                     }
{code}
- Any reason not to pass only List<Mutation>tableMetadata to addRowsToChildViews and
just add the new metadata rows in-place (iterating only up to the the original size)?
- Minor nit: how about just calling upgradeTo4_5_0(metaConnection) in the try and getting
rid of the if statement?
{code}
+                                
+                                if (currentServerSideTableTimeStamp < MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_5_0)
{
+                                    columnsToAdd = PhoenixDatabaseMetaData.BASE_COLUMN_COUNT
+ " "
+                                            + PInteger.INSTANCE.getSqlTypeName();
+                                    boolean skipUpgrade = false;
+                                    try {
+                                        addColumn(metaConnection, PhoenixDatabaseMetaData.SYSTEM_CATALOG,
+                                                MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP,
columnsToAdd, false);
+                                    } catch (ColumnAlreadyExistsException ignored) {
+                                        /* 
+                                         * Upgrade to 4.5 is a slightly special case. We
use the fact that the column
+                                         * BASE_COLUMN_COUNT is already part of the meta-data
schema as the signal that
+                                         * the server side upgrade has finished or is in
progress.
+                                         */
+                                        skipUpgrade = true;
+                                    }
+                                    if (!skipUpgrade) {
+                                        upgradeTo4_5_0(metaConnection);
+                                    }
+                                }
{code}
- In QueryConstants, it's best to add new columns at the end, otherwise the column ordinal
positions of preceding SYSTEM.CATALOG columns will get increment (just in case folks are counting
on the ordinal position).
- Use your constant here:
{code}
@@ -253,18 +254,18 @@ public class PTableImpl implements PTable {
         return new PTableImpl(tenantId, schemaName, tableName, type, state, timeStamp, sequenceNumber,
pkName, bucketNum, columns, dataSchemaName,
                 dataTableName, indexes, isImmutableRows, physicalNames, defaultFamilyName,
                 viewExpression, disableWAL, multiTenant, storeNulls, viewType, viewIndexId,
-                indexType, PTableStats.EMPTY_STATS);
+                indexType, PTableStats.EMPTY_STATS, -1);
{code}
- Remove TODO here:
{code}
+                multiTenant, storeNulls, viewType, viewIndexId, indexType, baseColumnCount);
// TODO
{code}
- Not a huge deal, but can the array actually be null here?
{code}
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/ByteUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/ByteUtil.java
index 1e3516d..1f4a285 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/ByteUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/ByteUtil.java
@@ -253,13 +253,17 @@ public class ByteUtil {
     public static byte[] concat(byte[] first, byte[]... rest) {
         int totalLength = first.length;
         for (byte[] array : rest) {
-            totalLength += array.length;
+            if (array != null) {
+                totalLength += array.length;
+            }
{code}

> Support adding column to a table that has views
> -----------------------------------------------
>
>                 Key: PHOENIX-1504
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-1504
>             Project: Phoenix
>          Issue Type: Sub-task
>            Reporter: James Taylor
>            Assignee: Samarth Jain
>             Fix For: 5.0.0, 4.5.0
>
>         Attachments: PHOENIX-1504-wip.patch, PHOENIX-1504.patch
>
>
> We currently disallow any schema modifications to a table that has views. We should relax
that constraint and push the schema change as necessary out to all views.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message