-
Notifications
You must be signed in to change notification settings - Fork 3.7k
CASSANDRA-18112 Support manual secondary index selection at the CQL level #4249
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
base: trunk
Are you sure you want to change the base?
Conversation
Do we need some test cases for this pr? |
@Maxwell-Guo This is still in progress, but don't worry...there will be tests ;) |
e9d65b4
to
e201fc2
Compare
First real CI attempt: https://ci-cassandra.apache.org/view/patches/job/Cassandra-devbranch-5/343/ |
df3eb40
to
54e39a9
Compare
test/distributed/org/apache/cassandra/distributed/test/sai/IndexHintsDistributedTest.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/cql3/restrictions/MergedRestriction.java
Outdated
Show resolved
Hide resolved
@@ -292,11 +294,23 @@ public boolean needsFiltering(Index.Group indexGroup) | |||
} | |||
|
|||
@Override | |||
public Index findSupportingIndex(Iterable<Index> indexes) | |||
public Index findSupportingIndex(IndexRegistry indexRegistry, IndexHints indexHints) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can deduplicate the two versions of findSupportingIndex
and keep the changes done by CASSANDRA-19341 this way: adelapena@52fbc4b
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-18112 @co-authored-by ? @co-authored-by ?
e201fc2
to
4307fb5
Compare
@@ -109,7 +109,7 @@ public V1SSTableIndex(SSTableContext sstableContext, StorageAttachedIndex index) | |||
catch (Throwable t) | |||
{ | |||
FileUtils.closeQuietly(indexFiles); | |||
FileUtils.closeQuietly(sstableContext); | |||
FileUtils.closeQuietly(super.sstableContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll probably have to port this to 5.0 as well...but for now it's stabilizing tests for this patch...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, +1
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-18112
This work is based on datastax#1670 by @adelapena
(We'll indicate co-authors on commit here)