You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by m2je <gi...@git.apache.org> on 2018/07/28 17:35:30 UTC
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
GitHub user m2je opened a pull request:
https://github.com/apache/phoenix/pull/317
PHOENIX-3547 Supporting more number of indices per table.
Currently the number of indices per Phoenix table is bound to maximum of 65535 (java.lang.Short) which is a limitation for applications requiring to have unlimited number of indices.
This change will consider any new table created in Phoenix to support view index ids to be in the range of -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807 (java.lang.Long) which is undoubtably big enough to cover this requirement.
Any existing Phoenix table will still continue to support only maximum of 65535 of indices.
A new boolean column (USE_LONG_VIEW_INDEX BOOLEAN DEFAULT FALSE) is added to SYSTEM.CATALOG to specify each Phoenix table's support for large number of indices.
On each new Phoenix table creation the value for USE_LONG_VIEW_INDEX will be set to `true` while this value would be false for any existing table.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/m2je/phoenix PHOENIX-3547
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/317.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 #317
----
commit f1ffcf6b76a0a764df5529a03ba8dc42112dc5a9
Author: Mahdi Salarkia <ms...@...>
Date: 2018-07-28T05:27:46Z
PHOENIX-3547 Supporting more number of indices per table.
Currently the number of indices per Phoenix table is bound to maximum of 65535 (java.lang.Short) which is a limitation for applications requiring to have unlimited number of indices.
This change will consider any new table created in Phoenix to support view index ids to be in the range of -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807 (java.lang.Long) which is undoubtably big enough to cover this requirement.
Any existing Phoenix table will still continue to support only maximum of 65535 of indices.
A new boolean column (USE_LONG_VIEW_INDEX BOOLEAN DEFAULT FALSE) is added to SYSTEM.CATALOG to specify each Phoenix table's support for large number of indices.
On each new Phoenix table creation the value for USE_LONG_VIEW_INDEX will be set to `true` while this value would be false for any existing table.
----
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206717705
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java ---
@@ -1223,7 +1225,9 @@ public void readFields(DataInput input) throws IOException {
boolean hasViewIndexId = encodedIndexedColumnsAndViewId < 0;
if (hasViewIndexId) {
// Fixed length
- viewIndexId = new byte[MetaDataUtil.getViewIndexIdDataType().getByteSize()];
+ //Use leacy viewIndexIdType for clients older than 4.10 release
--- End diff --
done
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r208661657
--- Diff: phoenix-protocol/src/main/MetaDataService.proto ---
@@ -75,7 +76,8 @@ message MetaDataResponse {
repeated SharedTableState sharedTablesToDelete = 9;
optional PSchema schema = 10;
optional int64 autoPartitionNum = 11;
- optional int32 viewIndexId = 12;
--- End diff --
👍 Thanks for quick response
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206328431
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java ---
@@ -204,14 +205,19 @@ private void appendPKColumnValue(StringBuilder buf, byte[] range, Boolean isNull
range = ptr.get();
}
if (changeViewIndexId) {
- Short s = (Short) type.toObject(range);
- s = (short) (s + (-Short.MAX_VALUE));
- buf.append(s.toString());
+ PDataType viewIndexDataType = tableRef.getTable().getViewIndexType();
+ buf.append(getViewIndexValue(type, range, viewIndexDataType).toString());
} else {
Format formatter = context.getConnection().getFormatter(type);
buf.append(type.toStringLiteral(range, formatter));
}
}
+
+ private Long getViewIndexValue(PDataType type, byte[] range, PDataType viewIndexDataType){
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexDataType);
+ Object s = type.toObject(range);
+ return (useLongViewIndex ? (Long) s : (Short) s) - (useLongViewIndex ? Long.MAX_VALUE : Short.MAX_VALUE);
--- End diff --
Shouldn't the <type> argument be all that's necessary? Why is an additional type argument needed? The type as defined in the schema should be the right type, no?
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207661302
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java ---
@@ -3037,6 +3037,14 @@ protected PhoenixConnection upgradeSystemCatalogIfRequired(PhoenixConnection met
addViewIndexToParentLinks(metaConnection);
moveChildLinks(metaConnection);
}
+ if (currentServerSideTableTimeStamp < MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_5_1_0) {
+ metaConnection = addColumnsIfNotExists(
+ metaConnection,
+ PhoenixDatabaseMetaData.SYSTEM_CATALOG,
+ MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_5_1_0,
+ PhoenixDatabaseMetaData.USE_LONG_VIEW_INDEX + " "
--- End diff --
change from boolean to data type
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207660126
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java ---
@@ -1340,6 +1344,9 @@ public static IndexMaintainer fromProto(ServerCachingProtos.IndexMaintainer prot
maintainer.nIndexSaltBuckets = proto.getSaltBuckets();
maintainer.isMultiTenant = proto.getIsMultiTenant();
maintainer.viewIndexId = proto.hasViewIndexId() ? proto.getViewIndexId().toByteArray() : null;
+ maintainer.viewIndexType = proto.hasUseLongViewIndex()
--- End diff --
change to use data type
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207673751
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java ---
@@ -204,15 +205,19 @@ private void appendPKColumnValue(StringBuilder buf, byte[] range, Boolean isNull
range = ptr.get();
}
if (changeViewIndexId) {
- Short s = (Short) type.toObject(range);
- s = (short) (s + (-Short.MAX_VALUE));
--- End diff --
The offset is just for readability. Instead of displaying -32,768, we display 0. For shared indexes (i.e. indexes on views or local indexes), we do this. It's not necessary functionally.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206326382
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -1431,11 +1438,42 @@ private PTable getTable(RegionScanner scanner, long clientTimeStamp, long tableT
// server while holding this lock is a bad idea and likely to cause contention.
return PTableImpl.makePTable(tenantId, schemaName, tableName, tableType, indexState, timeStamp, tableSeqNum,
pkName, saltBucketNum, columns, parentSchemaName, parentTableName, indexes, isImmutableRows, physicalTables, defaultFamilyName,
- viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexId, indexType,
+ viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexType, viewIndexId, indexType,
rowKeyOrderOptimizable, transactionProvider, updateCacheFrequency, baseColumnCount,
indexDisableTimestamp, isNamespaceMapped, autoPartitionSeq, isAppendOnlySchema, storageScheme, encodingScheme, cqCounter, useStatsForParallelization);
}
+ private Long getViewIndexId(Cell[] tableKeyValues, PDataType viewIndexType) {
+ Cell viewIndexIdKv = tableKeyValues[VIEW_INDEX_ID_INDEX];
+ return viewIndexIdKv == null ? null :
+ decodeViewIndexId(viewIndexIdKv, viewIndexType);
+ }
+ /**
+ * check the value for {@value USE_LONG_VIEW_INDEX} and if its present consider viewIndexId as long otherwise
+ * read as short and convert it to long
+ *
+ * @param tableKeyValues
+ * @param viewIndexType
+ * @return
+ */
+ private Long decodeViewIndexId(Cell viewIndexIdKv, PDataType viewIndexType) {
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexType);
+ return new Long(
+ useLongViewIndex
+ ? viewIndexType.getCodec().decodeLong(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ : MetaDataUtil.getLegacyViewIndexIdDataType().getCodec().decodeShort(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ );
+ }
+
+ private PDataType getViewIndexType(Cell[] tableKeyValues) {
+ Cell useLongViewIndexKv = tableKeyValues[USE_LONG_VIEW_INDEX];
--- End diff --
I think we need to keep the single VIEW_INDEX_ID column and make sure it's type is defined (through a property) at create time (and not allow it to be changed). The issue isn't with the metadata, but with the row key of the rows of the table. In old tables, it'll be a short while for new tables it'll be a long. We don't want to have to rewrite the data.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207669858
--- Diff: phoenix-protocol/src/main/ServerCachingService.proto ---
@@ -62,6 +62,7 @@ message IndexMaintainer {
repeated ColumnInfo indexedColumnInfo = 19;
required int32 encodingScheme = 20;
required int32 immutableStorageScheme = 21;
+ optional bool useLongViewIndex = 22;
--- End diff --
change to data type
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207670259
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -2285,6 +2317,7 @@ public void createTable(RpcController controller, CreateTableRequest request,
builder.setReturnCode(MetaDataProtos.MutationCode.TABLE_NOT_FOUND);
if (indexId != null) {
builder.setViewIndexId(indexId);
--- End diff --
In case the create table request was made from an older client you need to set the viewIndexId as an int32 (as thats what the client is expecting). See my other comment about maintaing backward compatibility.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207669604
--- Diff: phoenix-protocol/src/main/PTable.proto ---
@@ -85,7 +85,7 @@ message PTable {
optional bytes viewStatement = 18;
repeated bytes physicalNames = 19;
optional bytes tenantId = 20;
- optional int32 viewIndexId = 21;
+ optional int64 viewIndexId = 21;
--- End diff --
Same comment as earlier, we need to support backward compatibility.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r205952450
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -2185,7 +2223,7 @@ public void createTable(RpcController controller, CreateTableRequest request,
throw sqlExceptions[0];
}
long seqValue = seqValues[0];
- if (seqValue > Short.MAX_VALUE) {
+ if (seqValue > Long.MAX_VALUE) {
--- End diff --
This check is no longer needed since the indexId is now a Long. When you call incrementSequences if the current sequence value is LONG.MAX_VALUE you will get an exception.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r205954264
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -1431,11 +1438,42 @@ private PTable getTable(RegionScanner scanner, long clientTimeStamp, long tableT
// server while holding this lock is a bad idea and likely to cause contention.
return PTableImpl.makePTable(tenantId, schemaName, tableName, tableType, indexState, timeStamp, tableSeqNum,
pkName, saltBucketNum, columns, parentSchemaName, parentTableName, indexes, isImmutableRows, physicalTables, defaultFamilyName,
- viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexId, indexType,
+ viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexType, viewIndexId, indexType,
rowKeyOrderOptimizable, transactionProvider, updateCacheFrequency, baseColumnCount,
indexDisableTimestamp, isNamespaceMapped, autoPartitionSeq, isAppendOnlySchema, storageScheme, encodingScheme, cqCounter, useStatsForParallelization);
}
+ private Long getViewIndexId(Cell[] tableKeyValues, PDataType viewIndexType) {
+ Cell viewIndexIdKv = tableKeyValues[VIEW_INDEX_ID_INDEX];
+ return viewIndexIdKv == null ? null :
+ decodeViewIndexId(viewIndexIdKv, viewIndexType);
+ }
+ /**
+ * check the value for {@value USE_LONG_VIEW_INDEX} and if its present consider viewIndexId as long otherwise
+ * read as short and convert it to long
+ *
+ * @param tableKeyValues
+ * @param viewIndexType
+ * @return
+ */
+ private Long decodeViewIndexId(Cell viewIndexIdKv, PDataType viewIndexType) {
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexType);
+ return new Long(
+ useLongViewIndex
+ ? viewIndexType.getCodec().decodeLong(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ : MetaDataUtil.getLegacyViewIndexIdDataType().getCodec().decodeShort(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ );
+ }
+
+ private PDataType getViewIndexType(Cell[] tableKeyValues) {
+ Cell useLongViewIndexKv = tableKeyValues[USE_LONG_VIEW_INDEX];
--- End diff --
For existing tables the VIEW_INDEX_ID column will store the index as a short while for new tables the value will be stored as a long.
Would it be cleaner to create a new column that stores the view index id as a long. In QueryServicesConnectionImpl.upgradeSystemTables() we would set the new column based on the existing value. Finally we can remove the existing VIEW_INDEX_ID in the next release.
WDYT @JamesRTaylor ?
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207659053
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -1431,11 +1438,42 @@ private PTable getTable(RegionScanner scanner, long clientTimeStamp, long tableT
// server while holding this lock is a bad idea and likely to cause contention.
return PTableImpl.makePTable(tenantId, schemaName, tableName, tableType, indexState, timeStamp, tableSeqNum,
pkName, saltBucketNum, columns, parentSchemaName, parentTableName, indexes, isImmutableRows, physicalTables, defaultFamilyName,
- viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexId, indexType,
+ viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexType, viewIndexId, indexType,
rowKeyOrderOptimizable, transactionProvider, updateCacheFrequency, baseColumnCount,
indexDisableTimestamp, isNamespaceMapped, autoPartitionSeq, isAppendOnlySchema, storageScheme, encodingScheme, cqCounter, useStatsForParallelization);
}
+ private Long getViewIndexId(Cell[] tableKeyValues, PDataType viewIndexType) {
+ Cell viewIndexIdKv = tableKeyValues[VIEW_INDEX_ID_INDEX];
+ return viewIndexIdKv == null ? null :
+ decodeViewIndexId(viewIndexIdKv, viewIndexType);
+ }
+ /**
+ * check the value for {@value USE_LONG_VIEW_INDEX} and if its present consider viewIndexId as long otherwise
+ * read as short and convert it to long
+ *
+ * @param tableKeyValues
+ * @param viewIndexType
+ * @return
+ */
+ private Long decodeViewIndexId(Cell viewIndexIdKv, PDataType viewIndexType) {
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexType);
--- End diff --
Just call getCodec().decodeLong() in both cases here.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r210127502
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java ---
@@ -1340,6 +1344,9 @@ public static IndexMaintainer fromProto(ServerCachingProtos.IndexMaintainer prot
maintainer.nIndexSaltBuckets = proto.getSaltBuckets();
maintainer.isMultiTenant = proto.getIsMultiTenant();
maintainer.viewIndexId = proto.hasViewIndexId() ? proto.getViewIndexId().toByteArray() : null;
+ maintainer.viewIndexType = proto.hasUseLongViewIndex()
--- End diff --
done
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r205952155
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java ---
@@ -1223,7 +1225,9 @@ public void readFields(DataInput input) throws IOException {
boolean hasViewIndexId = encodedIndexedColumnsAndViewId < 0;
if (hasViewIndexId) {
// Fixed length
- viewIndexId = new byte[MetaDataUtil.getViewIndexIdDataType().getByteSize()];
+ //Use leacy viewIndexIdType for clients older than 4.10 release
--- End diff --
nit: typo
---
[GitHub] phoenix issue #317: PHOENIX-3547 Supporting more number of indices per table...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on the issue:
https://github.com/apache/phoenix/pull/317
Thanks @twdsilva for the review and feed backs.
I'll wait for for your teams feed back on proposed change to VIEW_INDEX_ID column and soon after getting the confirmation, I'll update this PR.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207657327
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -317,6 +318,10 @@
private static final KeyValue MULTI_TENANT_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, MULTI_TENANT_BYTES);
private static final KeyValue VIEW_TYPE_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, VIEW_TYPE_BYTES);
private static final KeyValue VIEW_INDEX_ID_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, VIEW_INDEX_ID_BYTES);
+ /**
+ * A designator for choosing the right type for viewIndex (Short vs Long) to be backward compatible.
+ * **/
+ private static final KeyValue USE_LONG_VIEW_INDEX_ID_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, USE_LONG_VIEW_INDEX_BYTES);
--- End diff --
Instead of using a boolean can you use an int to store the data type. You can do something similar to how the DATA_TYPE of a column is set.
Cell dataTypeKv = colKeyValues[DATA_TYPE_INDEX];
PDataType dataType =
PDataType.fromTypeId(PInteger.INSTANCE.getCodec().decodeInt(
dataTypeKv.getValueArray(), dataTypeKv.getValueOffset(), SortOrder.getDefault()));
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r210127471
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java ---
@@ -393,9 +408,12 @@ public static MetaDataMutationResult constructFromProto(MetaDataResponse proto)
if (proto.hasAutoPartitionNum()) {
result.autoPartitionNum = proto.getAutoPartitionNum();
}
- if (proto.hasViewIndexId()) {
- result.viewIndexId = (short)proto.getViewIndexId();
- }
+ if (proto.hasViewIndexId()) {
+ result.viewIndexId = proto.getViewIndexId();
+ }
+ result.viewIndexType = proto.hasUseLongViewIndexId()
--- End diff --
done
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207662014
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java ---
@@ -2544,7 +2546,7 @@ else if (!SchemaUtil.isSystemTable(Bytes.toBytes(SchemaUtil.getTableName(schemaN
Collections.<PTable>emptyList(), isImmutableRows,
Collections.<PName>emptyList(), defaultFamilyName == null ? null :
PNameFactory.newName(defaultFamilyName), null,
- Boolean.TRUE.equals(disableWAL), false, false, null, null, indexType, true, null, 0, 0L, isNamespaceMapped, autoPartitionSeq, isAppendOnlySchema, ONE_CELL_PER_COLUMN, NON_ENCODED_QUALIFIERS, PTable.EncodedCQCounter.NULL_COUNTER, true);
+ Boolean.TRUE.equals(disableWAL), false, false, null, null, null, indexType, true, null, 0, 0L, isNamespaceMapped, autoPartitionSeq, isAppendOnlySchema, ONE_CELL_PER_COLUMN, NON_ENCODED_QUALIFIERS, PTable.EncodedCQCounter.NULL_COUNTER, true);
--- End diff --
can viewIndexType be used here instead of null
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r209101337
--- Diff: phoenix-protocol/src/main/MetaDataService.proto ---
@@ -75,7 +76,8 @@ message MetaDataResponse {
repeated SharedTableState sharedTablesToDelete = 9;
optional PSchema schema = 10;
optional int64 autoPartitionNum = 11;
- optional int32 viewIndexId = 12;
--- End diff --
From the doc it looks like it should be ok to just change the viewIndexId type from int32 to int64.
We don't have a good way to test that this in a unit test. You could test that this works correctly manually by creating a view with the 4.14 server and client jar and then replacing the server jar with one that has your patch and verifying that you can still run queries against the view.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207665609
--- Diff: phoenix-protocol/src/main/MetaDataService.proto ---
@@ -75,7 +76,8 @@ message MetaDataResponse {
repeated SharedTableState sharedTablesToDelete = 9;
optional PSchema schema = 10;
optional int64 autoPartitionNum = 11;
- optional int32 viewIndexId = 12;
+ optional int64 viewIndexId = 12;
+ optional bool useLongViewIndexId = 13;
--- End diff --
change from boolean to data type
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207661113
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java ---
@@ -204,15 +205,19 @@ private void appendPKColumnValue(StringBuilder buf, byte[] range, Boolean isNull
range = ptr.get();
}
if (changeViewIndexId) {
- Short s = (Short) type.toObject(range);
- s = (short) (s + (-Short.MAX_VALUE));
--- End diff --
@JamesRTaylor Do you know what this is for? It called form ExplainTable.appendScanRow and changeViewIndexId is true only for localIndexes and for the first element of the List<List<KeyRange>>.
Once viewIndexId is a long do we still need offset the id?
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206715551
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java ---
@@ -204,14 +205,19 @@ private void appendPKColumnValue(StringBuilder buf, byte[] range, Boolean isNull
range = ptr.get();
}
if (changeViewIndexId) {
- Short s = (Short) type.toObject(range);
- s = (short) (s + (-Short.MAX_VALUE));
- buf.append(s.toString());
+ PDataType viewIndexDataType = tableRef.getTable().getViewIndexType();
+ buf.append(getViewIndexValue(type, range, viewIndexDataType).toString());
} else {
Format formatter = context.getConnection().getFormatter(type);
buf.append(type.toStringLiteral(range, formatter));
}
}
+
+ private Long getViewIndexValue(PDataType type, byte[] range, PDataType viewIndexDataType){
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexDataType);
+ Object s = type.toObject(range);
+ return (useLongViewIndex ? (Long) s : (Short) s) - (useLongViewIndex ? Long.MAX_VALUE : Short.MAX_VALUE);
--- End diff --
Done
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207665512
--- Diff: phoenix-protocol/src/main/MetaDataService.proto ---
@@ -75,7 +76,8 @@ message MetaDataResponse {
repeated SharedTableState sharedTablesToDelete = 9;
optional PSchema schema = 10;
optional int64 autoPartitionNum = 11;
- optional int32 viewIndexId = 12;
--- End diff --
You cannot change the type of viewIndexId because we need to support an old client talking to a new server while a cluster is being upgraded. Add a new optional bytes field and file a JIRA to remove the viewIndexId field in the next release.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207659334
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -1431,11 +1438,42 @@ private PTable getTable(RegionScanner scanner, long clientTimeStamp, long tableT
// server while holding this lock is a bad idea and likely to cause contention.
return PTableImpl.makePTable(tenantId, schemaName, tableName, tableType, indexState, timeStamp, tableSeqNum,
pkName, saltBucketNum, columns, parentSchemaName, parentTableName, indexes, isImmutableRows, physicalTables, defaultFamilyName,
- viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexId, indexType,
+ viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexType, viewIndexId, indexType,
rowKeyOrderOptimizable, transactionProvider, updateCacheFrequency, baseColumnCount,
indexDisableTimestamp, isNamespaceMapped, autoPartitionSeq, isAppendOnlySchema, storageScheme, encodingScheme, cqCounter, useStatsForParallelization);
}
+ private Long getViewIndexId(Cell[] tableKeyValues, PDataType viewIndexType) {
+ Cell viewIndexIdKv = tableKeyValues[VIEW_INDEX_ID_INDEX];
+ return viewIndexIdKv == null ? null :
+ decodeViewIndexId(viewIndexIdKv, viewIndexType);
+ }
+ /**
+ * check the value for {@value USE_LONG_VIEW_INDEX} and if its present consider viewIndexId as long otherwise
+ * read as short and convert it to long
+ *
+ * @param tableKeyValues
+ * @param viewIndexType
+ * @return
+ */
+ private Long decodeViewIndexId(Cell viewIndexIdKv, PDataType viewIndexType) {
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexType);
+ return new Long(
+ useLongViewIndex
+ ? viewIndexType.getCodec().decodeLong(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ : MetaDataUtil.getLegacyViewIndexIdDataType().getCodec().decodeShort(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ );
+ }
+
+ private PDataType getViewIndexType(Cell[] tableKeyValues) {
+ Cell useLongViewIndexKv = tableKeyValues[USE_LONG_VIEW_INDEX];
--- End diff --
Instead of having a boolean add a column that represents the type of the view index id (VIEW_INDEX_ID_DATA_TYPE)
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r210123510
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -317,6 +318,10 @@
private static final KeyValue MULTI_TENANT_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, MULTI_TENANT_BYTES);
private static final KeyValue VIEW_TYPE_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, VIEW_TYPE_BYTES);
private static final KeyValue VIEW_INDEX_ID_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, VIEW_INDEX_ID_BYTES);
+ /**
+ * A designator for choosing the right type for viewIndex (Short vs Long) to be backward compatible.
+ * **/
+ private static final KeyValue USE_LONG_VIEW_INDEX_ID_KV = createFirstOnRow(ByteUtil.EMPTY_BYTE_ARRAY, TABLE_FAMILY_BYTES, USE_LONG_VIEW_INDEX_BYTES);
--- End diff --
done
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r209021243
--- Diff: phoenix-protocol/src/main/MetaDataService.proto ---
@@ -75,7 +76,8 @@ message MetaDataResponse {
repeated SharedTableState sharedTablesToDelete = 9;
optional PSchema schema = 10;
optional int64 autoPartitionNum = 11;
- optional int32 viewIndexId = 12;
--- End diff --
@twdsilva @JamesRTaylor
I just ran into this:
https://developers.google.com/protocol-buffers/docs/proto#updating
```
int32, uint32, int64, uint64, and bool are all compatible – this means you can change a field from one of these types to another without breaking forwards- or backwards-compatibility. If a number is parsed from the wire which doesn't fit in the corresponding type, you will get the same effect as if you had cast the number to that type in C++ (e.g. if a 64-bit number is read as an int32, it will be truncated to 32 bits)
```
If I understand it correctly, we can just add the `indexType` column to the .proto wherever we have viewIndexId and change the `viewIndexId` type from `int32` to `int64` without any side effect for old clients and hence we won't need to add `viewIndexLongId` and PHOENIX-4838.
Am I missing anything?
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je closed the pull request at:
https://github.com/apache/phoenix/pull/317
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r205953961
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java ---
@@ -204,14 +205,19 @@ private void appendPKColumnValue(StringBuilder buf, byte[] range, Boolean isNull
range = ptr.get();
}
if (changeViewIndexId) {
- Short s = (Short) type.toObject(range);
- s = (short) (s + (-Short.MAX_VALUE));
- buf.append(s.toString());
+ PDataType viewIndexDataType = tableRef.getTable().getViewIndexType();
+ buf.append(getViewIndexValue(type, range, viewIndexDataType).toString());
} else {
Format formatter = context.getConnection().getFormatter(type);
buf.append(type.toStringLiteral(range, formatter));
}
}
+
+ private Long getViewIndexValue(PDataType type, byte[] range, PDataType viewIndexDataType){
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexDataType);
+ Object s = type.toObject(range);
+ return (useLongViewIndex ? (Long) s : (Short) s) - (useLongViewIndex ? Long.MAX_VALUE : Short.MAX_VALUE);
--- End diff --
@JamesRTaylor or @chrajeshbabu do you know if this change is correct?
---
[GitHub] phoenix issue #317: PHOENIX-3547 Supporting more number of indices per table...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on the issue:
https://github.com/apache/phoenix/pull/317
moved to https://github.com/apache/phoenix/pull/324
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207661456
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java ---
@@ -192,7 +192,8 @@
DISABLE_WAL + " BOOLEAN,\n" +
MULTI_TENANT + " BOOLEAN,\n" +
VIEW_TYPE + " UNSIGNED_TINYINT,\n" +
- VIEW_INDEX_ID + " SMALLINT,\n" +
+ VIEW_INDEX_ID + " BIGINT,\n" +
+ USE_LONG_VIEW_INDEX + " BOOLEAN,\n" +
--- End diff --
change to data type
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r207659847
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java ---
@@ -393,9 +408,12 @@ public static MetaDataMutationResult constructFromProto(MetaDataResponse proto)
if (proto.hasAutoPartitionNum()) {
result.autoPartitionNum = proto.getAutoPartitionNum();
}
- if (proto.hasViewIndexId()) {
- result.viewIndexId = (short)proto.getViewIndexId();
- }
+ if (proto.hasViewIndexId()) {
+ result.viewIndexId = proto.getViewIndexId();
+ }
+ result.viewIndexType = proto.hasUseLongViewIndexId()
--- End diff --
change the boolean to use an int to represent the data type
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r208432236
--- Diff: phoenix-protocol/src/main/MetaDataService.proto ---
@@ -75,7 +76,8 @@ message MetaDataResponse {
repeated SharedTableState sharedTablesToDelete = 9;
optional PSchema schema = 10;
optional int64 autoPartitionNum = 11;
- optional int32 viewIndexId = 12;
--- End diff --
You are correct the new property can be an optional int64.
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206715585
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -2185,7 +2223,7 @@ public void createTable(RpcController controller, CreateTableRequest request,
throw sqlExceptions[0];
}
long seqValue = seqValues[0];
- if (seqValue > Short.MAX_VALUE) {
+ if (seqValue > Long.MAX_VALUE) {
--- End diff --
done
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206715609
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---
@@ -1431,11 +1438,42 @@ private PTable getTable(RegionScanner scanner, long clientTimeStamp, long tableT
// server while holding this lock is a bad idea and likely to cause contention.
return PTableImpl.makePTable(tenantId, schemaName, tableName, tableType, indexState, timeStamp, tableSeqNum,
pkName, saltBucketNum, columns, parentSchemaName, parentTableName, indexes, isImmutableRows, physicalTables, defaultFamilyName,
- viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexId, indexType,
+ viewStatement, disableWAL, multiTenant, storeNulls, viewType, viewIndexType, viewIndexId, indexType,
rowKeyOrderOptimizable, transactionProvider, updateCacheFrequency, baseColumnCount,
indexDisableTimestamp, isNamespaceMapped, autoPartitionSeq, isAppendOnlySchema, storageScheme, encodingScheme, cqCounter, useStatsForParallelization);
}
+ private Long getViewIndexId(Cell[] tableKeyValues, PDataType viewIndexType) {
+ Cell viewIndexIdKv = tableKeyValues[VIEW_INDEX_ID_INDEX];
+ return viewIndexIdKv == null ? null :
+ decodeViewIndexId(viewIndexIdKv, viewIndexType);
+ }
+ /**
+ * check the value for {@value USE_LONG_VIEW_INDEX} and if its present consider viewIndexId as long otherwise
+ * read as short and convert it to long
+ *
+ * @param tableKeyValues
+ * @param viewIndexType
+ * @return
+ */
+ private Long decodeViewIndexId(Cell viewIndexIdKv, PDataType viewIndexType) {
+ boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexType);
+ return new Long(
+ useLongViewIndex
+ ? viewIndexType.getCodec().decodeLong(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ : MetaDataUtil.getLegacyViewIndexIdDataType().getCodec().decodeShort(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(), SortOrder.getDefault())
+ );
+ }
+
+ private PDataType getViewIndexType(Cell[] tableKeyValues) {
+ Cell useLongViewIndexKv = tableKeyValues[USE_LONG_VIEW_INDEX];
--- End diff --
done
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Posted by m2je <gi...@git.apache.org>.
Github user m2je commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r208422561
--- Diff: phoenix-protocol/src/main/MetaDataService.proto ---
@@ -75,7 +76,8 @@ message MetaDataResponse {
repeated SharedTableState sharedTablesToDelete = 9;
optional PSchema schema = 10;
optional int64 autoPartitionNum = 11;
- optional int32 viewIndexId = 12;
--- End diff --
@twdsilva Just for clarification. Do you mean to add a new column like
`optional bytes viewIndexLongId = 13;`
and then retire viewIndexId in the next version.
This make sense but I only don't understand why the new property cannot be a optional int64?
---