-
Notifications
You must be signed in to change notification settings - Fork 22
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
CNDB-12739: Fix row filter ignoring distinct index and query analyzers #1548
Conversation
Checklist before you submit for review
|
3bf5cab
to
8f89950
Compare
test/distributed/org/apache/cassandra/distributed/test/sai/QueryAnalyzerDistributedTest.java
Outdated
Show resolved
Hide resolved
I understand we care the most about the test case added at the moment. But I want to point out a few things for follow-up:
While you check that I will verify also the other PR |
AbstractAnalyzer analyzer = factory.create(); | ||
try | ||
{ | ||
analyzer.reset(value); |
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.
Shouldn't this do the reset on a duplicate of value
?
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.
Probably yes. I guess it's not failing because the caller doesn't do anything with the value after calling this, but it looks risky anyway, so I've added the duplication.
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.
What you said makes sense to me. As we need to be very careful with this PR - @michaeljmarshall , third pair of eyes on this, please, if you have a moment?
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 agree that calling duplicate is the safe option here and unless we have a reason to believe it is a hotspot, we should just call duplicate for now.
test/distributed/org/apache/cassandra/distributed/test/sai/QueryAnalyzerDistributedTest.java
Outdated
Show resolved
Hide resolved
Changing the signature of |
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
@@ -93,7 +93,7 @@ public Set<ColumnMetadata> getAnalyzedColumns(IndexRegistry indexRegistry) | |||
|
|||
for (ColumnCondition condition : this) | |||
{ | |||
if (indexRegistry.getAnalyzerFor(condition.column, condition.operator).isPresent()) | |||
if (indexRegistry.getIndexAnalyzerFor(condition.column, condition.operator).isPresent()) |
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 is unrelated to the PR, so should not block anything, but I wonder if we should add a method that checks if the column is analyzed instead of this getter, which allocates at least 2 objects per analyzer.
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.
+1 on clean CI (still running), thank you
bedf927
to
c280efd
Compare
|
❌ Build ds-cassandra-pr-gate/PR-1548 rejected by Butler1 new test failure(s) in 4 builds Found 1 new test failures
Found 7 known test failures |
Make
RowFilter.Expression
consider the two different analyzers that an index can have, one for write time and the other for read time.