You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by m2je <gi...@git.apache.org> on 2018/08/21 17:42:44 UTC
[GitHub] phoenix pull request #334: PHOENIX-3547 Supporting more number of indices pe...
GitHub user m2je opened a pull request:
https://github.com/apache/phoenix/pull/334
PHOENIX-3547 Supporting more number of indices per table.
Currently the number of indices per Phoenix table is bound to maximum of 65,535 (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 undoubtedly 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 viewIndex 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.
According to Protobuf documentation https://developers.google.com/protocol-buffers/docs/proto#updating we can change the type of viewIndexId from int32 to int64 and maintain the backward compatibility for older clients. We did a manual verification for this scenario by
creating a view with the old client and verify the new client is able to connect and read/write to the view after changing the viewIndexId type and adding the viewIndexType.
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/334.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 #334
----
commit 4b39872603b801b7c477dceb778cffc9475f6704
Author: Mahdi Salarkia <ms...@...>
Date: 2018-08-21T17:38:49Z
PHOENIX-3547 Supporting more number of indices per table.
Currently the number of indices per Phoenix table is bound to maximum of 65,535 (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 undoubtedly 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 viewIndex 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.
According to Protobuf documentation https://developers.google.com/protocol-buffers/docs/proto#updating we can change the type of viewIndexId from int32 to int64 and maintain the backward compatibility for older clients. We did a manual verification for this scenario by
creating a view with the old client and verify the new client is able to connect and read/write to the view after changing the viewIndexId type and adding the viewIndexType.
----
---
[GitHub] phoenix issue #334: 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/334
@twdsilva I can see the Jenkins job is complaining about the some lines being bigger than 100 chars. Many of those had been there and my commit just changed part of it. I can fix them all but not sure if this is expected from this PR.
Also I see some tests that had been reported to fail but they pass on retries.
Can you you give me an update on the status and what are things expected here.
Thanks
---
[GitHub] phoenix issue #334: 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/334
@twdsilva Added all patches to the ticket. Unfortunately they seem to all need to have their own patch except `4.x-cdh5.12`, `4.x-cdh5.13`,`4.x-cdh5.14`. But I added a patch for each anyway.
I have also captured the work here in case you want me to open pull requests for review purposes or comparing changes.
* https://github.com/apache/phoenix/compare/4.x-HBase-0.98...m2je:4.x-HBase-0.98-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-HBase-1.1...m2je:4.x-HBase-1.1-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-HBase-1.2...m2je:4.x-HBase-1.2-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-HBase-1.3...m2je:4.x-HBase-1.3-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-HBase-1.4...m2je:4.x-HBase-1.4-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-cdh5.11...m2je:4.x-cdh5.11-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-cdh5.12...m2je:4.x-cdh5.12-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-cdh5.13...m2je:4.x-cdh5.13-PHOENIX-3547
* https://github.com/apache/phoenix/compare/4.x-cdh5.14...m2je:4.x-cdh5.14-PHOENIX-3547
---
[GitHub] phoenix pull request #334: 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/334#discussion_r213047429
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java ---
@@ -436,16 +455,18 @@ public static MetaDataResponse toProto(MetaDataMutationResult result) {
sharedTableStateBuilder.setSchemaName(ByteStringer.wrap(sharedTableState.getSchemaName().getBytes()));
sharedTableStateBuilder.setTableName(ByteStringer.wrap(sharedTableState.getTableName().getBytes()));
sharedTableStateBuilder.setViewIndexId(sharedTableState.getViewIndexId());
+ sharedTableStateBuilder.setViewIndexType(sharedTableState.viewIndexType.getSqlType());
builder.addSharedTablesToDelete(sharedTableStateBuilder.build());
}
}
if (result.getSchema() != null) {
builder.setSchema(PSchema.toProto(result.schema));
}
builder.setAutoPartitionNum(result.getAutoPartitionNum());
- if (result.getViewIndexId() != null) {
- builder.setViewIndexId(result.getViewIndexId());
- }
+ if (result.getViewIndexId() != null) {
+ builder.setViewIndexId(result.getViewIndexId());
+ builder.setViewIndexType(result.getViewIndexType().getSqlType());
--- End diff --
done
---
[GitHub] phoenix pull request #334: 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/334
---
[GitHub] phoenix pull request #334: 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/334#discussion_r212805320
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/UnionCompiler.java ---
@@ -100,7 +100,7 @@ public static TableRef contructSchemaTable(PhoenixStatement statement, List<Quer
UNION_SCHEMA_NAME, UNION_TABLE_NAME, PTableType.SUBQUERY, null,
HConstants.LATEST_TIMESTAMP, scn == null ? HConstants.LATEST_TIMESTAMP : scn,
null, null, projectedColumns, null, null, null, true, null, null, null, true,
- true, true, null, null, null, false, null, 0, 0L,
+ true, true, null,null, null, null, false, null, 0, 0L,
--- End diff --
nit: space before null
---
[GitHub] phoenix pull request #334: 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/334#discussion_r213047491
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/UnionCompiler.java ---
@@ -100,7 +100,7 @@ public static TableRef contructSchemaTable(PhoenixStatement statement, List<Quer
UNION_SCHEMA_NAME, UNION_TABLE_NAME, PTableType.SUBQUERY, null,
HConstants.LATEST_TIMESTAMP, scn == null ? HConstants.LATEST_TIMESTAMP : scn,
null, null, projectedColumns, null, null, null, true, null, null, null, true,
- true, true, null, null, null, false, null, 0, 0L,
+ true, true, null,null, null, null, false, null, 0, 0L,
--- End diff --
done
---
[GitHub] phoenix pull request #334: 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/334#discussion_r212805302
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java ---
@@ -436,16 +455,18 @@ public static MetaDataResponse toProto(MetaDataMutationResult result) {
sharedTableStateBuilder.setSchemaName(ByteStringer.wrap(sharedTableState.getSchemaName().getBytes()));
sharedTableStateBuilder.setTableName(ByteStringer.wrap(sharedTableState.getTableName().getBytes()));
sharedTableStateBuilder.setViewIndexId(sharedTableState.getViewIndexId());
+ sharedTableStateBuilder.setViewIndexType(sharedTableState.viewIndexType.getSqlType());
builder.addSharedTablesToDelete(sharedTableStateBuilder.build());
}
}
if (result.getSchema() != null) {
builder.setSchema(PSchema.toProto(result.schema));
}
builder.setAutoPartitionNum(result.getAutoPartitionNum());
- if (result.getViewIndexId() != null) {
- builder.setViewIndexId(result.getViewIndexId());
- }
+ if (result.getViewIndexId() != null) {
+ builder.setViewIndexId(result.getViewIndexId());
+ builder.setViewIndexType(result.getViewIndexType().getSqlType());
--- End diff --
Can you add a null check for result.getViewIndexType()?
---
[GitHub] phoenix issue #334: 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/334
Thanks @twdsilva
Much appreciate your help. Will close this PR then.
---
[GitHub] phoenix issue #334: 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/334
@twdsilva Sure, The only issue I'm seeing is the each 4.x branch also need a separated patches. Let me prepare them.
---
[GitHub] phoenix issue #334: 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/334
@twdsilva all done 👍
---
[GitHub] phoenix issue #334: PHOENIX-3547 Supporting more number of indices per table...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/334
@m2je Thank you for the contribution. I have committed this patch to the 4.x and master branches.
For future reference you only need to create a single PR. Once it is approved please follow instructions on how to generate a patch from the website (https://phoenix.apache.org/contributing.html), and attach the patches to the JIRA.
---
[GitHub] phoenix issue #334: PHOENIX-3547 Supporting more number of indices per table...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/334
@m2je I was able to apply this PR to the 4.x branches, but it didn't apply cleanly to the master branch. Can you please attach a patch the the JIRA that applies cleanly to the master branch ?
---