-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-11762: Remove StorageAttachedIndexQueryPlan#postProcessor #1422
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
7ebea0e
to
7dd6ad7
Compare
7dd6ad7
to
18f1a4b
Compare
it looks like the whole postReconciliationProcessing -> postProcessor chain can be removed since the only remaining postProcessor is a no-op? |
Probably yes, since I think we don't have 3rd party 2i implementations using it for sorting. I suspect that in the long term sorting will be better placed at the same level as |
714c5fb
to
83e9777
Compare
83e9777
to
6c21586
Compare
6c21586
to
d7d9421
Compare
|
❌ Build ds-cassandra-pr-gate/PR-1422 rejected by Butler4 new test failure(s) in 7 builds Found 4 new test failures
Found 38 known test failures |
@michaeljmarshall will you have time to review this? It should be a nice performance improvement for coordinators. |
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! Sorry, I missed reviewing this sooner. I'm glad we'll be able to skip an extra sorting step in the coordinator.
The two test failures labeled as regressions are for detected leaks. I commented on https://github.com/riptano/cndb/issues/11910 with a reference to this PR. They are not related to this PR, so I am going to merge this now. |
Thanks for reviewing and merging :). There wasn't a PR for adding this to CNDB yet, so I have created this one: https://github.com/riptano/cndb/pull/12076 That PR also includes CNDB-11950, for which I haven't found the matching PR bumping the CNDB version. |
This reverts commit c06c94c. It seems the removal of `Index#postProcessor` by CNDB-11762 broke some tests in CNDB's `MultiNodeBillingTest`. Unfortunately that patch [was merged before creating the PR bumping the CC version used by CNDB](#1422 (comment)). [The CNDB PR](riptano/cndb#12076) was created after that merging but it was superseded by other CC version bumps. So I'm adding this reversal so we can investigate how the removal of `Index#postProcessor` affects those tests.
`StorageAttachedIndexQueryPlan#postProcessor` seems redundant because coordinator-side sorting+triming of ANN query results is already done in `SelectStatement`. If that's the case, we should remove it. - [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version - [ ] Use `NoSpamLogger` for log lines that may appear frequently in the logs - [ ] Verify test results on Butler - [ ] Test coverage for new/modified code is > 80% - [ ] Proper code formatting - [ ] Proper title for each commit staring with the project-issue number, like CNDB-1234 - [ ] Each commit has a meaningful description - [ ] Each commit is not very long and contains related changes - [ ] Renames, moves and reformatting are in distinct commits
This reverts commit c06c94c. It seems the removal of `Index#postProcessor` by CNDB-11762 broke some tests in CNDB's `MultiNodeBillingTest`. Unfortunately that patch [was merged before creating the PR bumping the CC version used by CNDB](#1422 (comment)). [The CNDB PR](riptano/cndb#12076) was created after that merging but it was superseded by other CC version bumps. So I'm adding this reversal so we can investigate how the removal of `Index#postProcessor` affects those tests.
`StorageAttachedIndexQueryPlan#postProcessor` seems redundant because coordinator-side sorting+triming of ANN query results is already done in `SelectStatement`. If that's the case, we should remove it. - [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version - [ ] Use `NoSpamLogger` for log lines that may appear frequently in the logs - [ ] Verify test results on Butler - [ ] Test coverage for new/modified code is > 80% - [ ] Proper code formatting - [ ] Proper title for each commit staring with the project-issue number, like CNDB-1234 - [ ] Each commit has a meaningful description - [ ] Each commit is not very long and contains related changes - [ ] Renames, moves and reformatting are in distinct commits
This reverts commit c06c94c. It seems the removal of `Index#postProcessor` by CNDB-11762 broke some tests in CNDB's `MultiNodeBillingTest`. Unfortunately that patch [was merged before creating the PR bumping the CC version used by CNDB](#1422 (comment)). [The CNDB PR](riptano/cndb#12076) was created after that merging but it was superseded by other CC version bumps. So I'm adding this reversal so we can investigate how the removal of `Index#postProcessor` affects those tests.
`StorageAttachedIndexQueryPlan#postProcessor` seems redundant because coordinator-side sorting+triming of ANN query results is already done in `SelectStatement`. If that's the case, we should remove it. - [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version - [ ] Use `NoSpamLogger` for log lines that may appear frequently in the logs - [ ] Verify test results on Butler - [ ] Test coverage for new/modified code is > 80% - [ ] Proper code formatting - [ ] Proper title for each commit staring with the project-issue number, like CNDB-1234 - [ ] Each commit has a meaningful description - [ ] Each commit is not very long and contains related changes - [ ] Renames, moves and reformatting are in distinct commits
This reverts commit c06c94c. It seems the removal of `Index#postProcessor` by CNDB-11762 broke some tests in CNDB's `MultiNodeBillingTest`. Unfortunately that patch [was merged before creating the PR bumping the CC version used by CNDB](#1422 (comment)). [The CNDB PR](riptano/cndb#12076) was created after that merging but it was superseded by other CC version bumps. So I'm adding this reversal so we can investigate how the removal of `Index#postProcessor` affects those tests.
`StorageAttachedIndexQueryPlan#postProcessor` seems redundant because coordinator-side sorting+triming of ANN query results is already done in `SelectStatement`. If that's the case, we should remove it. - [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version - [ ] Use `NoSpamLogger` for log lines that may appear frequently in the logs - [ ] Verify test results on Butler - [ ] Test coverage for new/modified code is > 80% - [ ] Proper code formatting - [ ] Proper title for each commit staring with the project-issue number, like CNDB-1234 - [ ] Each commit has a meaningful description - [ ] Each commit is not very long and contains related changes - [ ] Renames, moves and reformatting are in distinct commits
This reverts commit c06c94c. It seems the removal of `Index#postProcessor` by CNDB-11762 broke some tests in CNDB's `MultiNodeBillingTest`. Unfortunately that patch [was merged before creating the PR bumping the CC version used by CNDB](#1422 (comment)). [The CNDB PR](riptano/cndb#12076) was created after that merging but it was superseded by other CC version bumps. So I'm adding this reversal so we can investigate how the removal of `Index#postProcessor` affects those tests.
`StorageAttachedIndexQueryPlan#postProcessor` seems redundant because coordinator-side sorting+triming of ANN query results is already done in `SelectStatement`. If that's the case, we should remove it. - [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version - [ ] Use `NoSpamLogger` for log lines that may appear frequently in the logs - [ ] Verify test results on Butler - [ ] Test coverage for new/modified code is > 80% - [ ] Proper code formatting - [ ] Proper title for each commit staring with the project-issue number, like CNDB-1234 - [ ] Each commit has a meaningful description - [ ] Each commit is not very long and contains related changes - [ ] Renames, moves and reformatting are in distinct commits
This reverts commit c06c94c. It seems the removal of `Index#postProcessor` by CNDB-11762 broke some tests in CNDB's `MultiNodeBillingTest`. Unfortunately that patch [was merged before creating the PR bumping the CC version used by CNDB](#1422 (comment)). [The CNDB PR](riptano/cndb#12076) was created after that merging but it was superseded by other CC version bumps. So I'm adding this reversal so we can investigate how the removal of `Index#postProcessor` affects those tests.
StorageAttachedIndexQueryPlan#postProcessor
seems redundant because coordinator-side sorting+triming of ANN query results is already done inSelectStatement
. If that's the case, we should remove it.Checklist before you submit for review
NoSpamLogger
for log lines that may appear frequently in the logs