PHOENIX-3547 Supporting more number of indices per table. #317

m2je · 2018-07-28T17:35:27Z

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 int column (VIEW_INDEX_ID_DATA_TYPE TINYINT) is added to SYSTEM.CATALOG to specify each Phoenix table's vewIndex data type.
On each new Phoenix table creation the value for VIEW_INDEX_ID_DATA_TYPE will be set to Long while this value would be Short for any existing 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.

twdsilva · 2018-07-28T19:36:12Z

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


twdsilva · 2018-07-28T19:52:35Z

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


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.

twdsilva · 2018-07-28T21:06:34Z

phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java

+    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);


@JamesRTaylor or @chrajeshbabu do you know if this change is correct?

Shouldn't the 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?

twdsilva · 2018-07-28T21:22:37Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+    }
+
+    private PDataType getViewIndexType(Cell[] tableKeyValues) {
+        Cell useLongViewIndexKv = tableKeyValues[USE_LONG_VIEW_INDEX];


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 ?

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.

Instead of having a boolean add a column that represents the type of the view index id (VIEW_INDEX_ID_DATA_TYPE)

m2je · 2018-07-30T20:58:41Z

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.

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.

twdsilva

Instead of using a boolean to represent whether the view index id is a long or not use an int to represent the view index data type.

We need to handle an older client talking to a new server while a cluster is being upgraded. While changing the protobuf you need to ensure backward compatiblity is maintained.

twdsilva · 2018-08-03T20:22:31Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+    /**
+     * 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);


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()));

twdsilva · 2018-08-03T20:29:05Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+     * @return
+     */
+    private Long decodeViewIndexId(Cell viewIndexIdKv,  PDataType viewIndexType) {
+        boolean useLongViewIndex = MetaDataUtil.getViewIndexIdDataType().equals(viewIndexType);


Just call getCodec().decodeLong() in both cases here.

twdsilva · 2018-08-03T20:30:06Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

+    }
+
+    private PDataType getViewIndexType(Cell[] tableKeyValues) {
+        Cell useLongViewIndexKv = tableKeyValues[USE_LONG_VIEW_INDEX];


Instead of having a boolean add a column that represents the type of the view index id (VIEW_INDEX_ID_DATA_TYPE)

twdsilva · 2018-08-03T20:32:14Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java

+          if (proto.hasViewIndexId()) {
+              result.viewIndexId = proto.getViewIndexId();
+          }
+          result.viewIndexType = proto.hasUseLongViewIndexId()


change the boolean to use an int to represent the data type

twdsilva · 2018-08-03T20:33:27Z

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


change to use data type

twdsilva · 2018-08-03T20:56:43Z

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;


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.

@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?

You are correct the new property can be an optional int64.

👍 Thanks for quick response

@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?

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.

twdsilva · 2018-08-03T20:57:08Z

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;


change from boolean to data type

twdsilva · 2018-08-03T21:15:09Z

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;


Same comment as earlier, we need to support backward compatibility.

twdsilva · 2018-08-03T21:16:28Z

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;


change to data type

twdsilva · 2018-08-03T21:18:35Z

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


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.

…til.Base64

… Zhang)

Closes apache#318 Signed-off-by: Josh Elser <elserj@apache.org>

…changes

… specific logger

…ist to ASC sort order always

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 int column (VIEW_INDEX_ID_DATA_TYPE TINYINT) is added to SYSTEM.CATALOG to specify each Phoenix table's vewIndex data type. On each new Phoenix table creation the value for VIEW_INDEX_ID_DATA_TYPE will be set to `Long` while this value would be `Short` for any existing table.

m2je · 2018-08-14T22:49:19Z

moved to #324

twdsilva reviewed Jul 28, 2018

View reviewed changes

twdsilva requested changes Aug 3, 2018

View reviewed changes

twdsilva and others added 12 commits August 14, 2018 15:04

PHOENIX-4824 Update BRANCH_NAMES in dev/test-patch.properties

8b270b8

PHOENIX-4825 Replace usage of HBase Base64 implementation with java.u…

1b24570

…til.Base64

PHOENIX-4826 Changes to support HBase 2.0.1

0126a93

PHOENIX-4822 Ensure the provided timezone is used client-side (Jaanai…

38021c7

… Zhang)

PHOENIX-4822 Fixed Spelling.

9846a6c

Closes apache#318 Signed-off-by: Josh Elser <elserj@apache.org>

PHOENIX-4751 Implement client-side hash aggregation

fe9c079

PHOENIX-4799 Write cells using checkAndMutate to prevent conflicting …

d190517

…changes

PHOENIX-4834 PhoenixMetricsLog interface methods should not depend on…

332c9aa

… specific logger

PHOENIX-4805 Move Avatica version to 1.12 for PQS

9d2175b

PHOENIX-4647 Column header doesn't handle optional arguments correctly

e8e604e

PHOENIX-4843 InListExpression toString() converts the values in the l…

6ae5cb0

…ist to ASC sort order always

m2je closed this Aug 14, 2018

m2je deleted the PHOENIX-3547 branch August 21, 2018 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-3547 Supporting more number of indices per table. #317

PHOENIX-3547 Supporting more number of indices per table. #317

m2je commented Jul 28, 2018 •

edited

Loading

twdsilva Jul 28, 2018

m2je Jul 31, 2018

twdsilva Jul 28, 2018

m2je Jul 31, 2018

twdsilva Jul 28, 2018

JamesRTaylor Jul 30, 2018

m2je Jul 31, 2018

twdsilva Jul 28, 2018

JamesRTaylor Jul 30, 2018

m2je Jul 31, 2018

twdsilva Aug 3, 2018

m2je commented Jul 30, 2018

twdsilva left a comment

twdsilva Aug 3, 2018

m2je Aug 14, 2018

twdsilva Aug 3, 2018

twdsilva Aug 3, 2018

twdsilva Aug 3, 2018

m2je Aug 14, 2018

twdsilva Aug 3, 2018

m2je Aug 14, 2018

twdsilva Aug 3, 2018

m2je Aug 8, 2018

twdsilva Aug 8, 2018

m2je Aug 8, 2018

m2je Aug 9, 2018

twdsilva Aug 9, 2018

twdsilva Aug 3, 2018

twdsilva Aug 3, 2018

twdsilva Aug 3, 2018

twdsilva Aug 3, 2018

m2je commented Aug 14, 2018

PHOENIX-3547 Supporting more number of indices per table. #317

PHOENIX-3547 Supporting more number of indices per table. #317

Conversation

m2je commented Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m2je commented Jul 30, 2018

twdsilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m2je commented Aug 14, 2018

m2je commented Jul 28, 2018 •

edited

Loading