Skip to content

PHOENIX-1734 Local index improvements #135

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

Closed
wants to merge 2 commits into from
Closed

PHOENIX-1734 Local index improvements #135

wants to merge 2 commits into from

Conversation

chrajeshbabu
Copy link
Contributor

Patch supports storing local indexing data in the same data table.

  1. Removed code used HBase internals in balancer, split and merge.
  2. Create index create column families suffix with L# for data column families.
  3. Changes in read and write path to use column families prefixed with L# for local indexes.
  4. Done changes to tests.

@@ -186,7 +186,9 @@ private void testDeleteRange(boolean autoCommit, boolean createIndex, boolean lo
PreparedStatement stmt;
conn.setAutoCommit(autoCommit);
deleteStmt = "DELETE FROM IntIntKeyTest WHERE i >= ? and i < ?";
assertIndexUsed(conn, deleteStmt, Arrays.<Object>asList(5,10), indexName, false);
if(!local) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't the local index used here and below anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be used. In case of local indexes explain plan always have "SCAN OVER DATATABLENAME" so it's failing so not checking assertion for local indexes case. Will modify it make proper assertion in case of local indexes like it should have index id as well.

@chrajeshbabu
Copy link
Contributor Author

Thanks for review @JamesRTaylor. Please find my answers to the comments.

@JamesRTaylor
Copy link
Contributor

Couple of other comments:

  • You'll need to add code during installation of 4.7.0 (in ConnectionQueryServicesImpl.init()) that takes care of removing old coprocessors, adding new coprocessors, disabling local indexes and potentially automatically kicking off a new index build
  • We'll likely want to add a check that disallows column family names prefixed with "C#" to prevent inadvertently treating these as local index shadow column families. If we can get rid of the checks that use this prefix, then we might not need to do this.
  • Also, there's an edge case of an existing column family using the "C#" prefix. Unlikely, but not sure how we'd want to handle this. Maybe error on an attempt to create a local index on such a table?

@stoty
Copy link
Contributor

stoty commented Sep 28, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 0s Docker mode activated.
-1 ❌ patch 0m 3s #135 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/in-progress/precommit-patchnames for help.
Subsystem Report/Notes
GITHUB PR #135
Console output https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-135/1/console
versions git=2.17.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants