-
Notifications
You must be signed in to change notification settings - Fork 886
JAVA-913: Redesign IndexMetadata API. #452
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
Conversation
b10e9bb to
94b8dad
Compare
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.
This one is failing on <= 2.1 for some reason, though looks like it should work...looking into it.
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.
Just needed to selectively create the index based on major version much like should_create_metadata_for_index_on_map_values, i.e.: https://gist.github.com/tolbertam/4be3efec48975fe4b4d4
a9f08b3 to
d56dd0b
Compare
|
Added a version requirement check to one of the tests (feel free to squash that when merging). This looks good to me 👍 |
|
Added another test (for testing with multiple indexes on same column - not really necessary but figured would be good to add) and also added 'kind' validation. PR still looks good to me 👍 |
|
I noticed via cassandra-dtests that it was possible to add secondary indexes to materialized views. I noticed that the driver didn't support that yet, so i added it here in 92dabb3 since it seemed pretty straightforward, feel free to back out/change/etc. if it isn't an appropriate change. Secondary indexes are semi-busted on C* trunk right now (added comment to CASSANDRA-9921), so the test I added currently fails (unless 8000 you put a breakpoint before the execute statement and add the index manually twice), but should pass once fixed in C*. |
|
lgtm 👍 thanks for finding out that 2is on mvs are allowed! |
|
@tolbertam I just pushed a commit reverting indexes on materialized views following the decision in CASSANDRA-9921. Could you kindly review it again? |
|
Sounds good, taking a look :) |
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.
It'd probably be fine to just delete this test and if support gets added back we can readd it then
|
looks good to me 👍 |
This commit contains substantial contributions by Andrew Tolbert (@tolbertam).
JAVA-913: Redesign IndexMetadata API.
See CASSANDRA-10216 for details.