cassandra icon indicating copy to clipboard operation
cassandra copied to clipboard

CASSANDRA-19620: Refactor ColumnCondition

Open blerer opened this issue 1 year ago • 1 comments

The patch is a follow up from CASSANDRA-19341. It moves some of the ColumnCondition logic into Operator and ColumnExpression in order to simplify the logic needed to addition new operators and ensure that they are supported everywhere by Cassandra.

blerer avatar May 24 '24 14:05 blerer

Not a full review yet.

  1. https://github.com/apache/cassandra/pull/3331/commits/1070c8a3c437260b5d49e4c6cbae60f31d9ab835— Caleb suggested we verify that the merge of the bounds is okay for Accord (in regards to CASSANDRA-18100). You are in the chat with Ariel. I will look into that big commit once we confirm it.
  2. The other two commits - the simplifications look great. Thank you!
  3. Did you start fixing some of the null handling? Shouldn't we leave this for the other ticket?
  4. Checked the CI results - all seem to be known failures

ekaterinadimitrova2 avatar May 29 '24 21:05 ekaterinadimitrova2

According to GitHub these were all the changes done since the last time I reviewed before rebase: https://github.com/apache/cassandra/compare/be5994550feb605e22b0d1f9e42f4f90e63d0b38..093edabe4fe28abfc4174c29a364f2d0b071c28e The changes are minor and cosmetic, and my questions were addressed in the PR. CI doesn't show any issues on rebase, and the tests that were changed and failing are now all passing after CASSANDRA-19637/

I will proofread the whole final patch, but at the moment, it seems we are ready.

ekaterinadimitrova2 avatar Jul 01 '24 18:07 ekaterinadimitrova2

Was the test coverage for the isSatisfiedBy methods verified? https://github.com/apache/cassandra/pull/3331#pullrequestreview-2091386258

ekaterinadimitrova2 avatar Jul 01 '24 18:07 ekaterinadimitrova2

LGTM, we can commit it if CI also thinks it LGTI :-)

ekaterinadimitrova2 avatar Jul 23 '24 18:07 ekaterinadimitrova2