Skip to content
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

Fix EqBehavior bug introduced by #1434 #1614

Merged
merged 2 commits into from
Feb 27, 2025
Merged

Fix EqBehavior bug introduced by #1434 #1614

merged 2 commits into from
Feb 27, 2025

Conversation

michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Feb 27, 2025

What is the issue

The NON_DAEMON IndexRegistry returns true for all calls to supportsExpression. This leads to an incorrect result from the getEqBehavior method where we get MATCH instead of EQ because the index indicates that it can handle ANALYZER_MATCHES expressions and EQ expressions.

It is an odd edge case because the javadoc for the NON_DAEMON object is:

    /**
     * An {@code IndexRegistry} intended for use when Cassandra is initialized in client or tool mode.
     * Contains a single stub {@code Index} which possesses no actual indexing or searching capabilities
     * but enables query validation and preparation to succeed. Useful for tools which need to prepare
     * CQL statements without instantiating the whole ColumnFamilyStore infrastructure.
     */

This presents a problem for the eq/match logic where we want to find a nuanced solution to the different solutions. My proposal here is to just make it use the EQ behavior, but that might have adverse side effects in untested code.

What does this PR fix and why was it fixed

The original fix used in #1434 was just to avoid the NPE we hit, but it allowed for the wrong result in eq behavior. This fix is to say that we should just return EQ in that case. It's possible this fix has negative consequences that we haven't seen yet, but at the very least, the CNDB tests pass with it.

…metadata so code around that"

This commit incorrectly addresses the
issue of a null IndexMetadata
returned by the NON_DAEMON index registry.
The specific issue is that it returns
MATCH instead of EQ for the behavior.

This reverts commit 8268866.
@michaeljmarshall michaeljmarshall self-assigned this Feb 27, 2025
Copy link

Checklist before you submit for review

  • 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

@cassci-bot
Copy link

❌ Build ds-cassandra-pr-gate/PR-1614 rejected by Butler


1 new test failure(s) in 1 builds
See build details here


Found 1 new test failures

Test Explanation Branch history Upstream history
o.a.c.u.b.BinLogTest.testTruncationReleasesLogS... regression 🔴 🔵🔵🔵🔵🔵🔵🔵

No known test failures found

@michaeljmarshall michaeljmarshall merged commit 3f00247 into main Feb 27, 2025
472 of 477 checks passed
@michaeljmarshall michaeljmarshall deleted the fix-bm25 branch February 27, 2025 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants