apache
CASSANDRA-19947: Add constraints framework
Add new Constraints framework as described in CEP-42. This initial Jira ticket includes the core constraints length and numeric. Follow up tickets will include the rest of the constraints described in the CEP.
patch by Bernardo Botella; reviewed by <Reviewers> for CASSANDRA-19947
The Cassandra Jira
My feedback on parser.g. https://github.com/yifan-c/cassandra/commit/1d78e80f364f7dd70d5c76298def125f478afe95
- removed some redundant definitions
- permit using check and length as column names
Great work on this patch everyone; very impressed with it. I had a few follow up thoughts / questions after my 1st pass at review that I didn't add in the review comments:
- in-jvm dtest: Mixed version clusters (some nodes older w/out feature, some with)
- in-jvm dtest: Mixed feature enabled clusters (some nodes with feature / constraints enabled, some without). i.e. a case where schema didn't propagate on a change or raced w/someone altering a table w/constraints. This one might not be necessary in a post-TCM world actually; happy to learn if so. :)
- I don't recall seeing logic when I reviewed the patch to defer to a Guardrail being set in the event you tried to add a Constraint on that existing table. Did I miss that, or did we drop that (believe that was in the CEP)
- Was there unit testing around the unprepared json additions for constraint checking and I missed that?
Thanks everyone for the feedback and comments @jmckenzie-dev @Maxwell-Guo @smiklosovic ! I think I addressed all the comments that were left on the code.
As per @jmckenzie-dev questions, let's go one by one.
- That's interesting! Can you point me to other part of the code where we are doing such tests? I'd like to take a look at them as an example to have something that covers this scenario.
- So, I don't think this feature will go out there in a release that won't come up with TCM as well right? Should we defer this scenario?
- The CEP mentions interaction with currently preexisting guardrails. I don't think it proposes having the feature hidden behind a guardrail?
- Json is covered with the integration tests in
testConstraintWithJsonInsert.
hello, @bbotella , do you think we need to add some test for DescribeStatementTest to see if the out put of the describe table is what we need ? Besides, Do we need cqlsh to support the ability to automatically complete the corresponding keywords for constraints and add the corresponding unit test? like here . @smiklosovic @jmckenzie-dev WDYT ?
@Maxwell-Guo There is already tests covering the describe statement in the CreateTableWithColumnCqlConstraintValidationTest test class. Here:
https://github.com/apache/cassandra/pull/3562/files#diff-bb537e06c36f3dc9cfb0d52cfcbd75b609b51f5696c13f93f467a4b9fc79f483R50
I like the suggestion for the autocompletion. I will look into it.
hello, @bbotella , do you think we need to add some test for DescribeStatementTest to see if the out put of the describe table is what we need ? Besides, Do we need cqlsh to support the ability to automatically complete the corresponding keywords for constraints and add the corresponding unit test? like here . @smiklosovic @jmckenzie-dev WDYT ?
I know there's some on the project that would advocate for adding a test to ensure the output of DESCRIBE TABLE meets some very ultra-specific string output for every given new feature or change. I've never been too convinced of the necessity of that, but adding something to DescribeStatementTest.java to cover the constraint output case would cover it if you want to.
Honestly, we probably should do things like that and this is something I just need to come around on. I find those kinds of tests a real slog to both write and to debug (just annoying nit-picky string formatting stuff that snapshots the state of things at a moment in time, etc)
I will take another pass, since there are considerable amount of changes since my last review.