Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

m2je
Copy link

@m2je m2je commented Aug 21, 2018

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.

@m2je
Copy link
Author

m2je commented Aug 23, 2018

@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

Copy link
Contributor

@twdsilva twdsilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m2je No need to fix existing lines longer than 100 chars. I requested two minor changes, other than that this PR looks good to me. Can you make those two changes and I will get this committed?

}
if (result.getViewIndexId() != null) {
builder.setViewIndexId(result.getViewIndexId());
builder.setViewIndexType(result.getViewIndexType().getSqlType());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a null check for result.getViewIndexType()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space before null

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@m2je m2je force-pushed the PHOENIX-3547 branch 2 times, most recently from 338584f to 0708ae9 Compare August 27, 2018 17:10
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.
@m2je
Copy link
Author

m2je commented Aug 27, 2018

@twdsilva all done 👍

@twdsilva
Copy link
Contributor

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

@m2je
Copy link
Author

m2je commented Aug 30, 2018

@twdsilva Sure, The only issue I'm seeing is the each 4.x branch also need a separated patches. Let me prepare them.

@m2je
Copy link
Author

m2je commented Aug 31, 2018

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

@twdsilva
Copy link
Contributor

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

@m2je
Copy link
Author

m2je commented Aug 31, 2018

Thanks @twdsilva
Much appreciate your help. Will close this PR then.

@m2je m2je closed this Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants