-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-15774: Fix the CQL query validation to allow SAI indexes with OR operators when regular indexes present #2094
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
Checklist before you submit for review
|
…when regular indexes present Previously, when a table had both a SAI index (supporting OR operators) and a regular index (not supporting OR operators), queries with OR operators would fail even if the query targeted the SAI-indexed column. This patch modifies the validation to pass if at least one index supports the given filter.
ecf1099 to
0a06ffa
Compare
adelapena
left a comment
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. A couple of nits that I failed to detect on the DSP ticket are the comment about both SAI and secondary indexes and the PR and commit title itself. The fix is not in the query planner but in CQL validation, so a better commit description would be Fix CQL query validation to allow SAI indexes with OR operators when regular indexes are present. These two nits can be addressed on merge.
| @Test | ||
| public void shouldPickIndexWithDisjunctionSupportToServeTheQuery() | ||
| { | ||
| // given schema with both SAI and secondary indexes |
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.
Nit: both SAI and legacy/table-based is a secondary index, so I'd change this to something:
| // given schema with both SAI and secondary indexes | |
| // given schema with both SAI and legacy secondary indexes |
or:
| // given schema with both SAI and secondary indexes | |
| // given schema with both SAI and table-based secondary indexes |
or:
| // given schema with both SAI and secondary indexes | |
| // given schema with both SAI and cf-based secondary indexes |
|
That's right, in the DSP ticket the query planner was modified as well given I have ported a related change from CC, for this PR it's not the case anymore, good catch thank you! |
|
❌ Build ds-cassandra-pr-gate/PR-2094 rejected by Butler1 regressions found Found 1 new test failures
No known test failures found |
…exes with OR operators when regular indexes present (#2094) Previously, when a table had both a SAI index (supporting OR operators) and a regular index (not supporting OR operators), queries with OR operators would fail even if the query targeted the SAI-indexed column. This patch modifies the validation to pass if at least one index supports the given filter. CNDB PR: riptano/cndb#15865
…exes with OR operators when regular indexes present (#2094) Previously, when a table had both a SAI index (supporting OR operators) and a regular index (not supporting OR operators), queries with OR operators would fail even if the query targeted the SAI-indexed column. This patch modifies the validation to pass if at least one index supports the given filter. CNDB PR: riptano/cndb#15865



What is the issue
Previously, when a table had both a SAI index (supporting OR operators) and a regular index (not supporting OR operators), queries with OR operators would fail even if the query targeted the SAI-indexed column.
What does this PR fix and why was it fixed
This patch modifies the validation to pass if at least one index supports the given filter.
CNDB PR: https://github.com/riptano/cndb/pull/15865