[
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)
