-
Notifications
You must be signed in to change notification settings - Fork 37
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 duplicate of equal fields in the query when do range scanning with multiple clustering keys in Cassandra adapter #328
Conversation
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.
Overall LGTM! I just left minor comment and suggestion. Thank you!
core/src/main/java/com/scalar/db/storage/cassandra/SelectStatementHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/cassandra/SelectStatementHandler.java
Show resolved
Hide resolved
…ub.com/scalar-labs/scalardb into fix-cassandra-scan-query-with-mul-ckey
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! Thank you!
… fix-cassandra-scan-query-with-mul-ckey
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! Thank you!
I left one minor suggestion about the naming.
Also, I noticed some redundancy that we can improve in setStart/setEnd
and bindStart/bindEnd
.
It would be nice if you can make it less redundant in another PR if possible.
@@ -157,13 +161,15 @@ private void setStart(Select.Where statement, Scan scan) { | |||
statement.and(gt(start.get(i).getName(), bindMarker())); | |||
} | |||
} else { | |||
statement.and(eq(start.get(i).getName(), bindMarker())); | |||
String cKeyName = start.get(i).getName(); |
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.
String cKeyName = start.get(i).getName(); | |
String clusteringKeyName = start.get(i).getName(); |
Yeah, it's a bit long... but I think it's more clear. (otherwise we need to think what c
stands for).
Please update the other parts.
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.
Thanks. Applied in commit
…h multiple clustering keys in Cassandra adapter (#328)
…h multiple clustering keys in Cassandra adapter (#328)
…h multiple clustering keys in Cassandra adapter (#328)
When do integration testing with multiple clustering keys, I found that with the current implementation, it conducts duplicate of the equal fields in the query when doing range scanning with provided start and end clustering keys so got failed from Cassandra. For example, the wrong query is
SELECT * FROM integration_testing_FLOAT.test_table_mul_key_FLOAT_BIGINT WHERE c1=? AND c2=? AND c3>=? AND c2=? AND c3<=? ORDER BY c2 ASC,c3 ASC;
should be corrected to
SELECT * FROM integration_testing_FLOAT.test_table_mul_key_FLOAT_BIGINT WHERE c1=? AND c2=? AND c3>=? AND c3<=? ORDER BY c2 ASC,c3 ASC;
(remove one
AND c2=?
)This PR was made for fixing it.
Please take a look. Thank you.