Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-12373

3.0 breaks CQL compatibility with super columns families

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Fix Version/s: 3.0.15, 3.11.1, 4.0
    • Component/s: CQL
    • Labels:
      None

      Description

      This is a follow-up to CASSANDRA-12335 to fix the CQL side of super column compatibility.

      The details and a proposed solution can be found in the comments of CASSANDRA-12335 but the crux of the issue is that super column famillies show up differently in CQL in 3.0.x/3.x compared to 2.x, hence breaking backward compatibilty.

        Issue Links

          Activity

          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          I've started collecting information on what needs to be done. I just want to clarify the behaviour first:

          We would like to change the way schema and the resultset are currently represented (instead of the "" map<key_type, value_type> to two actual columns: column<number> (depending on the current clustering key size) and value, just as it was presented in example in CASSANDRA-12335, although preserve their internal representation (internally, map type will still be used for storage).

          In CQL terms

          CREATE TABLE tbl (
          	key ascii,
          	column1 ascii,
          	"" map<int, ascii>,
          	PRIMARY KEY (key, column1))
          	AND COMPACT STORAGE
          

          would return results in form of

           key          | column1 | column2 | value  |
          --------------+---------+---------+--------+
           key1         | val1    | 1       | value1 |
           key1         | val1    | 2       | value2 |
           key1         | val1    | 3       | value3 |
           key1         | val2    | 1       | value1 |
           key1         | val2    | 2       | value2 |
           key1         | val2    | 3       | value3 |
          

          (note that column2 is not clustering as Sylvain Lebresne described in comment).

          And this kind of special-casing will be valid for both read and write paths.

          Show
          ifesdjeen Alex Petrov added a comment - - edited I've started collecting information on what needs to be done. I just want to clarify the behaviour first: We would like to change the way schema and the resultset are currently represented (instead of the "" map<key_type, value_type> to two actual columns: column<number> (depending on the current clustering key size) and value , just as it was presented in example in CASSANDRA-12335 , although preserve their internal representation (internally, map type will still be used for storage). In CQL terms CREATE TABLE tbl ( key ascii, column1 ascii, "" map< int , ascii>, PRIMARY KEY (key, column1)) AND COMPACT STORAGE would return results in form of key | column1 | column2 | value | --------------+---------+---------+--------+ key1 | val1 | 1 | value1 | key1 | val1 | 2 | value2 | key1 | val1 | 3 | value3 | key1 | val2 | 1 | value1 | key1 | val2 | 2 | value2 | key1 | val2 | 3 | value3 | (note that column2 is not clustering as Sylvain Lebresne described in comment). And this kind of special-casing will be valid for both read and write paths.
          Hide
          slebresne Sylvain Lebresne added a comment -

          We would like to change the way schema and the resultset are currently represented

          Actually, we don't want to touch the schema. That is, to be precise, this ticket shouldn't change how anything is stored internally, and shouldn't thus change the schema tables. This does mean that fixing the output of DESCRIBE is actually not a direct part of this ticket, as I believe it's implemented by the python nowadays. We would however encourage drivers to special case super column familes too so that they expose tbl table of your example as:

          CREATE TABLE tbl (
                  key ascii,
                  column1 ascii,
                  column2 int,
                  value ascii,
                  PRIMARY KEY (key, column1, column2)
          ) WITH COMPACT STORAGE;
          

          and that's indeed how we want the table to behave.
          would return results in form of

          would return results in form of

          Yes, that's what we want. But this goes beyond just result-sets, we want the table to behave exactly as if it was the definition from above, namely that we'll allow queries like

          INSERT INTO tbl (key, column1, column2, value) VALUES (...);
          SELECT value FROM tbl WHERE key = 'key1' AND column1 = 'val1' AND column2 = 2;
          

          but we will not allow

          INSERT INTO tbl (key, column1, "") VALUES (....);
          SELECT "" FROM tbl WHERE key = 'key1' AND column1 = 'val1';
          

          In general though, the best description of what we want this ticket to do is that any CQL query on a super column table should behave in 3.0/3.x exactly as it behaved in 2.x. Which highlight the fact that we have no CQL tests for super columns, and a first step could be to write a decent coverage and test it on 2.x. And then we get it to work on 3.0/3.x.

          I'll note that as I said in CASSANDRA-12335, this means we'll probably need to intercept INSERT/UPDATE and SELECT (raw) statements on super column table early and basically rewrite them to match the internal representation, plus post-processing result sets. It would be really nice if we could keep all that code reasonably encapsulated too.

          Show
          slebresne Sylvain Lebresne added a comment - We would like to change the way schema and the resultset are currently represented Actually, we don't want to touch the schema. That is, to be precise, this ticket shouldn't change how anything is stored internally, and shouldn't thus change the schema tables. This does mean that fixing the output of DESCRIBE is actually not a direct part of this ticket, as I believe it's implemented by the python nowadays. We would however encourage drivers to special case super column familes too so that they expose tbl table of your example as: CREATE TABLE tbl ( key ascii, column1 ascii, column2 int, value ascii, PRIMARY KEY (key, column1, column2) ) WITH COMPACT STORAGE; and that's indeed how we want the table to behave. would return results in form of would return results in form of Yes, that's what we want. But this goes beyond just result-sets, we want the table to behave exactly as if it was the definition from above, namely that we'll allow queries like INSERT INTO tbl (key, column1, column2, value) VALUES (...); SELECT value FROM tbl WHERE key = 'key1' AND column1 = 'val1' AND column2 = 2; but we will not allow INSERT INTO tbl (key, column1, "") VALUES (....); SELECT "" FROM tbl WHERE key = 'key1' AND column1 = 'val1'; In general though, the best description of what we want this ticket to do is that any CQL query on a super column table should behave in 3.0/3.x exactly as it behaved in 2.x. Which highlight the fact that we have no CQL tests for super columns, and a first step could be to write a decent coverage and test it on 2.x. And then we get it to work on 3.0/3.x. I'll note that as I said in CASSANDRA-12335 , this means we'll probably need to intercept INSERT/UPDATE and SELECT (raw) statements on super column table early and basically rewrite them to match the internal representation, plus post-processing result sets. It would be really nice if we could keep all that code reasonably encapsulated too.
          Hide
          ifesdjeen Alex Petrov added a comment -

          Actually, we don't want to touch the schema.

          Right. I've tried to fix my wording (you might have seen the edits), but it was still imprecise.

          Thank you for confirming the results format. I'm mostly done with SELECT special-casing, just need to run a bit more tests to make sure that all the cases are covered. Will move to adding 2.x tests and then to INSERT/UPDATE.

          It would be really nice if we could keep all that code reasonably encapsulated too.

          Gladly, most of time we just need a ResultSet, Partition and CFMetaData, so keeping this code aside should not be a big problem. We could do it similar to CompactTables class.

          Show
          ifesdjeen Alex Petrov added a comment - Actually, we don't want to touch the schema. Right. I've tried to fix my wording (you might have seen the edits), but it was still imprecise. Thank you for confirming the results format. I'm mostly done with SELECT special-casing, just need to run a bit more tests to make sure that all the cases are covered. Will move to adding 2.x tests and then to INSERT/UPDATE . It would be really nice if we could keep all that code reasonably encapsulated too. Gladly, most of time we just need a ResultSet , Partition and CFMetaData , so keeping this code aside should not be a big problem. We could do it similar to CompactTables class.
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          What we want/need to do re:schema is change python- and java- drivers, but that's about it.

          Show
          iamaleksey Aleksey Yeschenko added a comment - What we want/need to do re:schema is change python- and java- drivers, but that's about it.
          Hide
          jjordan Jeremiah Jordan added a comment -

          Don't forget the "snapshot" schema changing code.

          Show
          jjordan Jeremiah Jordan added a comment - Don't forget the "snapshot" schema changing code.
          Hide
          slebresne Sylvain Lebresne added a comment -

          What we want/need to do re:schema is change python- and java- drivers

          Right, didn't meant nothing has to be done, just that it wasn't part of this patch (nor C* really). I'll mark "client-impacting" in fact, so clients are aware they have something to do here too.

          Don't forget the "snapshot" schema changing code.

          Well, that's kind of an interesting point because there is in fact currently no way to create a super column table from CQL. That's kind of on purpose, we do not want people to create them nowadays, we only want to maintain backward compatibility, but it does mean if you need to restore a snapshot of a super column table, you have to currently use thrift. So I don't known, maybe in the short term saying that we don't save schema for snapshot at all for super column table is the "most honest" solution.

          But it kind of suggest to me that we should get CASSANDRA-10857 in ASAP, and then force users to use it on super column tables before upgrading to 4.0. It's not ideal, but I don't dragging super columns support forever is a better solution either.

          Show
          slebresne Sylvain Lebresne added a comment - What we want/need to do re:schema is change python- and java- drivers Right, didn't meant nothing has to be done, just that it wasn't part of this patch (nor C* really). I'll mark "client-impacting" in fact, so clients are aware they have something to do here too. Don't forget the "snapshot" schema changing code. Well, that's kind of an interesting point because there is in fact currently no way to create a super column table from CQL. That's kind of on purpose, we do not want people to create them nowadays, we only want to maintain backward compatibility, but it does mean if you need to restore a snapshot of a super column table, you have to currently use thrift. So I don't known, maybe in the short term saying that we don't save schema for snapshot at all for super column table is the "most honest" solution. But it kind of suggest to me that we should get CASSANDRA-10857 in ASAP, and then force users to use it on super column tables before upgrading to 4.0. It's not ideal, but I don't dragging super columns support forever is a better solution either.
          Hide
          ifesdjeen Alex Petrov added a comment -

          While discussing CASSANDRA-7190, we decided to opt-out from writing custom schema loading format (and tool) and just CQL, so now we can only load things that are "CQL-expressible". In schema file we do "best effort", so we say "so here's what it kind of looks like internally, but there's actually no way to re-create that in CQL". We may just leave the schema file as is for now.

          Show
          ifesdjeen Alex Petrov added a comment - While discussing CASSANDRA-7190 , we decided to opt-out from writing custom schema loading format (and tool) and just CQL, so now we can only load things that are "CQL-expressible". In schema file we do "best effort", so we say "so here's what it kind of looks like internally, but there's actually no way to re-create that in CQL". We may just leave the schema file as is for now.
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          Sylvain Lebresne Aleksey Yeschenko do we want to support things like materialized views?

          (sorry about modification storm).

          Show
          ifesdjeen Alex Petrov added a comment - - edited Sylvain Lebresne Aleksey Yeschenko do we want to support things like materialized views? (sorry about modification storm).
          Hide
          ifesdjeen Alex Petrov added a comment -

          I've implemented a preliminary version of the patch (supercolum counters are still in progress):

          trunk dtest utest
          2.2 dtest utest
          dtest patch

          Should we include support for filtering, LWTs and materialized views? This is going to be some additional work because of column mappings / making fake columns available everywhere. Since we're bringing them back mostly to let people migrate to 3.x storage, I'd suggest leaving these three pieces out. What do you think?

          Show
          ifesdjeen Alex Petrov added a comment - I've implemented a preliminary version of the patch (supercolum counters are still in progress): trunk dtest utest 2.2 dtest utest dtest patch Should we include support for filtering, LWTs and materialized views? This is going to be some additional work because of column mappings / making fake columns available everywhere. Since we're bringing them back mostly to let people migrate to 3.x storage, I'd suggest leaving these three pieces out. What do you think?
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          Should we include support for filtering, LWTs and materialized views?

          The minimal requirement is to have the functionality present in 2.1.x/2.2.x restored - for people who migrated their supercolumn-using apps from Thrift to CQL and might already be using certain CQL features with those tables in their code. MVs weren't part of it, LWT and filtering - not sure.

          Show
          iamaleksey Aleksey Yeschenko added a comment - Should we include support for filtering, LWTs and materialized views? The minimal requirement is to have the functionality present in 2.1.x/2.2.x restored - for people who migrated their supercolumn-using apps from Thrift to CQL and might already be using certain CQL features with those tables in their code. MVs weren't part of it, LWT and filtering - not sure.
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          Thank you. LWTs didn't work, filtering didn't work either, although due to different reasons. I'll just add SuperColumn-specific error messages then.

          Show
          ifesdjeen Alex Petrov added a comment - - edited Thank you. LWTs didn't work , filtering didn't work either , although due to different reasons. I'll just add SuperColumn-specific error messages then.
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          I hope I've covered all the corner cases. A small note on the implementation: currently I'm using hardcoded column names column2 and value for the fake columns. If you think that can lead to some sort of collision or problem, we can try relying on column name generation from CompactTables, although it's going to be more or less same. These columns are not persisted together with the rest of schema, they're completely virtual and created during CFMetadata construction.

          As Sylvain Lebresne mentioned, SelectStatement, UpdateStatement and DeleteStatement were modified to special-case supercolumns, converting from map to two columns and reverse.

          trunk dtest utest
          3.X dtest utest
          2.2 dtest utest
          dtest patch
          Show
          ifesdjeen Alex Petrov added a comment - - edited I hope I've covered all the corner cases. A small note on the implementation: currently I'm using hardcoded column names column2 and value for the fake columns. If you think that can lead to some sort of collision or problem, we can try relying on column name generation from CompactTables , although it's going to be more or less same. These columns are not persisted together with the rest of schema, they're completely virtual and created during CFMetadata construction. As Sylvain Lebresne mentioned, SelectStatement , UpdateStatement and DeleteStatement were modified to special-case supercolumns, converting from map to two columns and reverse. trunk dtest utest 3.X dtest utest 2.2 dtest utest dtest patch
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          These columns are not persisted together with the rest of schema, they're completely virtual and created during CFMetadata construction.

          I do believe that in 2.1 we currently allow renaming these columns in CQL metadata. For users that have switched from Thrift to CQL for their supercolumns and changed from the default names, losing that metadata on upgrade might count as a breaking change.

          Show
          iamaleksey Aleksey Yeschenko added a comment - These columns are not persisted together with the rest of schema, they're completely virtual and created during CFMetadata construction. I do believe that in 2.1 we currently allow renaming these columns in CQL metadata. For users that have switched from Thrift to CQL for their supercolumns and changed from the default names, losing that metadata on upgrade might count as a breaking change.
          Hide
          ifesdjeen Alex Petrov added a comment -

          You're right, I've just checked and it is in fact possible to rename it since it's not a part of primary key.
          I hope that won't change the implementation a whole lot, we just need to pick up the columns during initialisation correctly. I'll run upgrade tests and check what could be the best way.

          Thanks for catching that.

          Show
          ifesdjeen Alex Petrov added a comment - You're right, I've just checked and it is in fact possible to rename it since it's not a part of primary key. I hope that won't change the implementation a whole lot, we just need to pick up the columns during initialisation correctly. I'll run upgrade tests and check what could be the best way. Thanks for catching that.
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          I've added a possible fix for that. In order to allow column renames, we have to change LegacySchemaMigrator to pass the columns from older versions in case of supercolumn family. In 3.0, however, they're removed from clustering and regular and converted back to fake "virtual" columns (with corresponding KIND and name, in order to avoid them popping up in queries. Already upgraded tables will still work, even though their schema doesn't have the second clustering and compact value columns, as they're initialised as fake columns if missing.

          Show
          ifesdjeen Alex Petrov added a comment - - edited I've added a possible fix for that. In order to allow column renames, we have to change LegacySchemaMigrator to pass the columns from older versions in case of supercolumn family. In 3.0, however, they're removed from clustering and regular and converted back to fake "virtual" columns (with corresponding KIND and name, in order to avoid them popping up in queries. Already upgraded tables will still work, even though their schema doesn't have the second clustering and compact value columns, as they're initialised as fake columns if missing.
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          Reviewing this next. Can you please rebase for most recent 3.X, just in case? Cheers.

          Show
          iamaleksey Aleksey Yeschenko added a comment - Reviewing this next. Can you please rebase for most recent 3.X, just in case? Cheers.
          Hide
          ifesdjeen Alex Petrov added a comment -

          Rebased all branches, links are updated above.

          Show
          ifesdjeen Alex Petrov added a comment - Rebased all branches, links are updated above.
          Hide
          ifesdjeen Alex Petrov added a comment -

          Previous patch had problems with renames of dense supercf (was a result of my assumption that renames would work
          similar to how they'd normally work in 3.x, although it's possible to rename all 4 columns in dense supercf and pk parts
          in sparse). Thanks Aleksey Yeschenko for clarifying sparse/dense semantics in the context of super-cf.

          Without renames, it was quite simple to sort out the supercolumn families, since the columns were always purely virtual.
          With renames, it got a bit more difficult and several problems appeared:

          • difference between sparse and dense causes trouble during upgrade (since in once case we have multiple
            regular rows, so we can't add the supercf key to regulars, since we won't be able to differentiate it later)
          • denseness calculation for supercf (because of the empty name map column) was causing errors during upgrade

          There were several other smaller things, all covered in the patch.

          3.X dtest utest
          2.2 dtest utest
          dtest patch

          I didn't update trunk because of the 4881d9c308ccd6b5ca70925bf6ebedb70e7705fc

          Show
          ifesdjeen Alex Petrov added a comment - Previous patch had problems with renames of dense supercf (was a result of my assumption that renames would work similar to how they'd normally work in 3.x, although it's possible to rename all 4 columns in dense supercf and pk parts in sparse). Thanks Aleksey Yeschenko for clarifying sparse/dense semantics in the context of super-cf. Without renames, it was quite simple to sort out the supercolumn families, since the columns were always purely virtual. With renames, it got a bit more difficult and several problems appeared: difference between sparse and dense causes trouble during upgrade (since in once case we have multiple regular rows, so we can't add the supercf key to regulars, since we won't be able to differentiate it later) denseness calculation for supercf (because of the empty name map column) was causing errors during upgrade There were several other smaller things, all covered in the patch. 3.X dtest utest 2.2 dtest utest dtest patch I didn't update trunk because of the 4881d9c308ccd6b5ca70925bf6ebedb70e7705fc
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          Tests LGTM, feel free to commit them separately - I think it'd be cleaner this way, anyway.

          Sill thinking through some potential edge cases for the main path on 3.X (to be applied to 3.0.X as well).

          Thanks.

          Show
          iamaleksey Aleksey Yeschenko added a comment - Tests LGTM, feel free to commit them separately - I think it'd be cleaner this way, anyway. Sill thinking through some potential edge cases for the main path on 3.X (to be applied to 3.0.X as well). Thanks.
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          I'll rebase on top of the latest branches as it seems like the patch has gotten a bit out of date.

          Show
          ifesdjeen Alex Petrov added a comment - - edited I'll rebase on top of the latest branches as it seems like the patch has gotten a bit out of date.
          Hide
          ifesdjeen Alex Petrov added a comment -

          Rebased on top of 3.0 and 3.11. Since we're doing this patch in the preparation for 4.0 where there'll be no thrift, supercolumnfamiles or compact tables, we do not need a trunk patch (only removing last bits of supercolumnfamilies and compact tables from code if there are any).

          3.0 3.11 dtest

          3.0 and 3.11 patches are quite similar but not exactly the same. In 3.0 there are fewer tests (due to the missing features) and there was a difference in SelectStatement since processPartitions is called from two places there. Not sure if we needed to abstract/hide it.

          CI results, including upgrade tests look good.

          Show
          ifesdjeen Alex Petrov added a comment - Rebased on top of 3.0 and 3.11 . Since we're doing this patch in the preparation for 4.0 where there'll be no thrift, supercolumnfamiles or compact tables, we do not need a trunk patch (only removing last bits of supercolumnfamilies and compact tables from code if there are any). 3.0 3.11 dtest 3.0 and 3.11 patches are quite similar but not exactly the same. In 3.0 there are fewer tests (due to the missing features) and there was a difference in SelectStatement since processPartitions is called from two places there. Not sure if we needed to abstract/hide it. CI results, including upgrade tests look good.
          Hide
          slebresne Sylvain Lebresne added a comment -

          The general approach lgtm, but I think there is problems around dealing with 'dense' versus 'non-dense'.

          Those things are a bit complicated however, and frankly under-documented, so allow me first to remind what 'dense' and 'non dense' mean for super column families (SCF in what follows), to make sure we're on the same page.

          The first important thing to note is that contrarily to other COMPACT STORAGE tables, the value of the is_dense flag doesn't impact the internal layout of a SCF, neither in 2.x nor in 3.x. What it does impact however (in 2.x so far at least) is how the SCF is exposed through CQL, and that's what we're trying to make work in this ticket (and it must work in the same way than in 2.x) .

          So the definition of being "dense" for a SCF in 2.x is that the user hasn't added any column in the thrift column_metadata of that SCF. Which equivalently means that is_dense == true if a SCF has no REGULAR columns internally.

          With that defined, the impact on CQL is the following:

          • a "dense" SCF having no REGULAR column, CQL exposes each "thrift column" of the SCF as an individual CQL row. So if you take a dense SCF containing something the following (using imaged representation of a thrift SCF here, hopefully it's clear what I mean):
            'k' : {
                'sc1' : {
                    'a' : 1,
                    'b' : 2,
                    'c' : 2,
                },
                'sc2': {
                    'b' : '3'
                }
            }
            

            then this is exposed in CQL as:

            key | column1 | column2 | value
            ----+---------+---------+-------
            'k' |   'sc1' |     'a' |     1
            'k' |   'sc1' |     'b' |     2
            'k' |   'sc1' |     'c' |     2
            'k' |   'sc2' |     'b' |     3
            
          • a "non dense" SCF however only exposes through CQL the values of "thrift columns" that belong to a defined thrift column_metadata. So considering the same SCF example, but saying that SCF is now non dense because the user has defined {{column_metadata=[ {column_name: b, validation_class: UTF8Type}

            ]}}, then that SCF will be exposed in CQL as

            key | column1 | b
            ----+---------+---
            'k' |   'sc1' | 2
            'k' |   'sc2' | 3
            

            Note in particular that any value not associated to a non-declared column_metadata is simply not exposed: it's there internally, but not accessible through CQL.

          I'll note at this point that the "why" this is done this way is unimportant here, we are way way past changing any of this. This is how things work in 2.x however and the only goal here is to make it work the same way in 3.x so user can upgrade without problems.

          Anyway, back to the patch, I think there is 2 problems:

          1. the methods in SuperColumnCompatibility don't seem to handle non-dense super columns properly. Typically, I haven't actually tested, but from reading the code I believe that for my example above (using the non dense case where 'b' is the only defined column) would yield something like:
            key | column1 | b
            ----+---------+---
            'k' |   'sc1' | 2
            'k' |   'sc1' | 2
            

            which is, well, not what 2.x does.

          2. I believe 3.x hasn't been setting the is_dense flag so far (which went unnoticed because, as said above, that flag only influence CQL in the case of SCF, and that hasn't working in 3.x so far). More precisely, I believe all SCF currently have their is_dense flag set to false in 3.x. And while the attached patch correctly fixes this issue for upgrades from 2.x, it's too late for cluster already upgraded to 3.x. And that's unfortunate because if I'm not mistaken, the schema migration process from 3.x has unintentionally erased the information we need to correct that problem. More precisely, the way to recognize a dense SCF in 2.1 is that is has no REGULAR column definitions internally, but dense SCF in 2.1 had a COMPACT_VALUE column definition, and in the 3.x schema migration code, we have converted that to REGULAR. In other words, if on 3.x we have a SCF with a single REGULAR column definition, we cannot really know with certainty if it's a dense SCF whose COMPACT_VALUE has been converted to a REGULAR, or a genuinely non-dense SCF with a single user-declared definition. I can't currently think of a good solution to that problem. We can play some guessing game using a few assumptions and hope no user will break those assumptions but I wanted to open up the discussion on that problem before delving into bad solutions in case someone has an actually good solution.

          Other than that, I only have a few very minor remarks:

          • I believe the last line of the class javadoc ("On write path, ...") of SuperColumnCompatibility has been truncated.
          • Hardcoding "column2" and "value" in SuperColumnCompatibility could theoretically clash with some use defined column names. There is code in CompactTables that's meant to deal with that and it probably make sense to reuse that.
          • Nit: Talking of CompactTables, there is a few super columns related stuffs, maybe it's worth moving some of that (possibly including the part of the class javadoc that explains SuperColumn) to SuperColumnCompatibility so it's easier to locate and most super column stuffs are in one place.
          • Nit: I'd probably merge getSuperCfKeyColumn() with makeSuperCfKeyColumn() and getSuperCfValueColumn() with makeSuperCfValueColumn(), as least as far as the "public" API of SuperColumnFamily is concerned (really just to keep things as encapsulated as possible).
          • Nit: Patch adds a few unused imports in SelectStatement.
          Show
          slebresne Sylvain Lebresne added a comment - The general approach lgtm, but I think there is problems around dealing with 'dense' versus 'non-dense'. Those things are a bit complicated however, and frankly under-documented, so allow me first to remind what 'dense' and 'non dense' mean for super column families (SCF in what follows), to make sure we're on the same page. The first important thing to note is that contrarily to other COMPACT STORAGE tables, the value of the is_dense flag doesn't impact the internal layout of a SCF, neither in 2.x nor in 3.x. What it does impact however (in 2.x so far at least) is how the SCF is exposed through CQL, and that's what we're trying to make work in this ticket (and it must work in the same way than in 2.x) . So the definition of being "dense" for a SCF in 2.x is that the user hasn't added any column in the thrift column_metadata of that SCF. Which equivalently means that is_dense == true if a SCF has no REGULAR columns internally. With that defined, the impact on CQL is the following: a "dense" SCF having no REGULAR column, CQL exposes each "thrift column" of the SCF as an individual CQL row. So if you take a dense SCF containing something the following (using imaged representation of a thrift SCF here, hopefully it's clear what I mean): 'k' : { 'sc1' : { 'a' : 1, 'b' : 2, 'c' : 2, }, 'sc2': { 'b' : '3' } } then this is exposed in CQL as: key | column1 | column2 | value ----+---------+---------+------- 'k' | 'sc1' | 'a' | 1 'k' | 'sc1' | 'b' | 2 'k' | 'sc1' | 'c' | 2 'k' | 'sc2' | 'b' | 3 a "non dense" SCF however only exposes through CQL the values of "thrift columns" that belong to a defined thrift column_metadata . So considering the same SCF example, but saying that SCF is now non dense because the user has defined {{column_metadata=[ {column_name: b, validation_class: UTF8Type} ]}}, then that SCF will be exposed in CQL as key | column1 | b ----+---------+--- 'k' | 'sc1' | 2 'k' | 'sc2' | 3 Note in particular that any value not associated to a non-declared column_metadata is simply not exposed: it's there internally, but not accessible through CQL. I'll note at this point that the "why" this is done this way is unimportant here, we are way way past changing any of this. This is how things work in 2.x however and the only goal here is to make it work the same way in 3.x so user can upgrade without problems. Anyway, back to the patch, I think there is 2 problems: the methods in SuperColumnCompatibility don't seem to handle non-dense super columns properly. Typically, I haven't actually tested, but from reading the code I believe that for my example above (using the non dense case where 'b' is the only defined column) would yield something like: key | column1 | b ----+---------+--- 'k' | 'sc1' | 2 'k' | 'sc1' | 2 which is, well, not what 2.x does. I believe 3.x hasn't been setting the is_dense flag so far (which went unnoticed because, as said above, that flag only influence CQL in the case of SCF, and that hasn't working in 3.x so far). More precisely, I believe all SCF currently have their is_dense flag set to false in 3.x. And while the attached patch correctly fixes this issue for upgrades from 2.x , it's too late for cluster already upgraded to 3.x. And that's unfortunate because if I'm not mistaken, the schema migration process from 3.x has unintentionally erased the information we need to correct that problem. More precisely, the way to recognize a dense SCF in 2.1 is that is has no REGULAR column definitions internally, but dense SCF in 2.1 had a COMPACT_VALUE column definition, and in the 3.x schema migration code, we have converted that to REGULAR . In other words, if on 3.x we have a SCF with a single REGULAR column definition, we cannot really know with certainty if it's a dense SCF whose COMPACT_VALUE has been converted to a REGULAR , or a genuinely non-dense SCF with a single user-declared definition. I can't currently think of a good solution to that problem. We can play some guessing game using a few assumptions and hope no user will break those assumptions but I wanted to open up the discussion on that problem before delving into bad solutions in case someone has an actually good solution. Other than that, I only have a few very minor remarks: I believe the last line of the class javadoc ("On write path, ...") of SuperColumnCompatibility has been truncated. Hardcoding "column2" and "value" in SuperColumnCompatibility could theoretically clash with some use defined column names. There is code in CompactTables that's meant to deal with that and it probably make sense to reuse that. Nit: Talking of CompactTables , there is a few super columns related stuffs, maybe it's worth moving some of that (possibly including the part of the class javadoc that explains SuperColumn) to SuperColumnCompatibility so it's easier to locate and most super column stuffs are in one place. Nit: I'd probably merge getSuperCfKeyColumn() with makeSuperCfKeyColumn() and getSuperCfValueColumn() with makeSuperCfValueColumn() , as least as far as the "public" API of SuperColumnFamily is concerned (really just to keep things as encapsulated as possible). Nit: Patch adds a few unused imports in SelectStatement .
          Hide
          jjordan Jeremiah Jordan added a comment -

          I'll note at this point that the "why" this is done this way is unimportant here, we are way way past changing any of this. This is how things work in 2.x however and the only goal here is to make it work the same way in 3.x so user can upgrade without problems.

          I agree we need to expose these the same way in 3.x, but one thing to remember is that in 2.x non SCF tables worked the same way, but in 3.x we started exposing the defined columns as "static" and the undefined ones as column1/value. Is there a similar way to expose all the data for SCF?

          Show
          jjordan Jeremiah Jordan added a comment - I'll note at this point that the "why" this is done this way is unimportant here, we are way way past changing any of this. This is how things work in 2.x however and the only goal here is to make it work the same way in 3.x so user can upgrade without problems. I agree we need to expose these the same way in 3.x, but one thing to remember is that in 2.x non SCF tables worked the same way, but in 3.x we started exposing the defined columns as "static" and the undefined ones as column1/value. Is there a similar way to expose all the data for SCF?
          Hide
          slebresne Sylvain Lebresne added a comment -

          but in 3.x we started exposing the defined columns as "static" and the undefined ones as column1/value.

          That's not exactly true. It's how we handle CS tables internally, but it's not how they are exposed. That's where CASSANDRA-10857 come in, as it's goal is exactly to allow exposing that internal layout and this will work for all compact tables, SCF included (and yes, all the data will be exposed this way).

          Show
          slebresne Sylvain Lebresne added a comment - but in 3.x we started exposing the defined columns as "static" and the undefined ones as column1/value. That's not exactly true. It's how we handle CS tables internally, but it's not how they are exposed. That's where CASSANDRA-10857 come in, as it's goal is exactly to allow exposing that internal layout and this will work for all compact tables, SCF included (and yes, all the data will be exposed this way).
          Hide
          jjordan Jeremiah Jordan added a comment -

          That's where CASSANDRA-10857 come in, as it's goal is exactly to allow exposing that internal layout and this will work for all compact tables, SCF included (and yes, all the data will be exposed this way).

          Show
          jjordan Jeremiah Jordan added a comment - That's where CASSANDRA-10857 come in, as it's goal is exactly to allow exposing that internal layout and this will work for all compact tables, SCF included (and yes, all the data will be exposed this way).
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          Sylvain Lebresne thank you for the review.

          I have fixed the problems with dense/non-dense supercolumns. It looks like from the CQL perspective non-dense SC tables are treated as if they were "normal" tables, since the internal map column isn't revealed through the CQL operations in any way. Of course, from the thrift perspective this is quite different. While going through the patch I've noticed several more problems, namely:

          • counters were not fully supported (they did work for the reads, however writes were not functional)
          • LWTs were not working
          • SELECT queries with the supercolumn key restriction was working incorrectly

          There were several more smaller fixes. All these things are now covered with tests. I've also updated the patch with your suggestions. Pushed the changes to the 3.0 branch to possibly save some cycles. 3.11 rebase should be relatively easy, with some minor changes. 2.2 version of branch contains updated tests to make the comparison with an older version simpler.

          Show
          ifesdjeen Alex Petrov added a comment - - edited Sylvain Lebresne thank you for the review. I have fixed the problems with dense/non-dense supercolumns. It looks like from the CQL perspective non-dense SC tables are treated as if they were "normal" tables, since the internal map column isn't revealed through the CQL operations in any way. Of course, from the thrift perspective this is quite different. While going through the patch I've noticed several more problems, namely: counters were not fully supported (they did work for the reads, however writes were not functional) LWTs were not working SELECT queries with the supercolumn key restriction was working incorrectly There were several more smaller fixes. All these things are now covered with tests. I've also updated the patch with your suggestions. Pushed the changes to the 3.0 branch to possibly save some cycles. 3.11 rebase should be relatively easy, with some minor changes. 2.2 version of branch contains updated tests to make the comparison with an older version simpler.
          Hide
          slebresne Sylvain Lebresne added a comment -

          Thanks for the update and great job on the much needed additional testing. I think we're getting there but I still have a bunch of remarks

          • In CFMetaData
            • In the ctor, for isDense, we seem to force recomputation for non-SC non-dense tables (which happens to include non-compact tables). I don't think thats what we want (and the recalculateIsDense is obviously meant only for SC). But for I'm also not 100% sure this work in all upgrade scenario. Typically, if a dense SCF is upgraded to a current 3.x version, I believe it will end up with 2 regular column definition: the first coming from the fact 2.x will have had a "COMPACT_VALUE" column definition, which will become a REGULAR when upgraded, and second being the "super column map" column that is force-added to all SCF. And so that re-computation code will not detect things properly. Note that I haven't actually tested this tbh so I could be wrong, but this highlight that we really need to be running some SC upgrade dtest both for 2.x -> 3.x+<this patch> and 2.x -> current 3.x -> 3.x+<this patch> because things are imo too complex to rely on reasoning alone. Unfortunately, while we were supposed to have a SC upgrade dtests, it appears this commit has made this test useless by making it (confusingly) not about super column families. So we'll need to revert that commit somehow and the improve the test to include both dense and sparse SC.
            • In rebuild(), why not move the SC code after columnMetadata has been populated so it can be simplified? Nothing seems to rely on it being done before columnMetadata is populated.
          • I'm not entirely sure if/how column renaming works for the SC key and value "fake" column (would have assumed AlterTableStatement would need a least a bit of special casing to recognize those "fake" column). It would be nice to add at test to check that renames still work like in 2.2 for those columns.
          • In UpdateParameters#addCounter, not sure the change from CounterContext#createUpdate to CounterContext#createLocal is corret. Probably a bad post-CASSANDRA-13691 rebase?
          • In {{SuperColumnCompatibility}:
            • The first line of the javadoc states that it's only for "dense" SC, but it's not 100% true as things like SUPER_COLUMN_MAP_COLUMN are used for all SC. And the rest of the javadoc describing the internal layout also applies to all SC. Bit of a detail I suppose, but I'd rephrase things to avoid any confusion.
            • In getSuperCfKeyColumn, not sure what "3.x-created supercolumn family" refers to (is that SC tables created from thrift in 3.x? or SC tables whose schema had been upgraded by a previous 3.x version?) and more importantly, why it would differ in number of clustering columns?
            • I'm confused by the code of getSuperCfValueColumn: dense tables are supposed to have only a single regular column, and cfm.compactValueColumn() is supposed to alias it. So I'm not sure what the loop is trying to do, except basically returning cfm.compactValueColumn(), nor when that loop wouldn't return (all SC should have the "super column map" internally. Further, due to what's above, said loop appears to return a definition whose type is the MapType, while the code following it clearly return a definition whose type is the values of said MapType, so it feels wrong the method would return different kind of types depending on the path taken. I'm most likely missing something since your tests seem to be working, but I'm not sure what.
            • In processPartition:
              • There is a typo in a comment, where in "... can't be change to support inclusive slice ...", I believe you want "exclusive".
              • In the REGULAR branch of the switch:
                • in the 2nd branch (else if ...), we should use result.add(cell) directly. Not only does that already handle counters, but more importantly, if we don't do that, things like ttl(v) will not work on the compact column value.
                • the last else is confusing: by definition we shouldn't be exposing any other column than the "compact value" column, so that branch should probably not be there (or be an assertion that we shouldn't get there).
            • In the updates loop in prepareUpdateOperations, not sure why the 2 first cases seem to assume that cfm.isSuperColumnValueColumn(def), but the 3rd case tests it explicitely (but after having assigned superColumnValue). In general, not sure why the 3rd case looks different from the other 2. Also, I find it a bit hard to follow the mix of if using continue and sometime using else, sometimes not. I'm personally prefer using else when needed and remove all the continue. Lastly, seems the method could use a check for superColumnKey != null.
            • In prepareInsertForJSONOperations, I believe the 2nd call to getRawTermForColumn is unecessary, you can use the raw for before the if. Further, the overall code of this method feels very similar to the one in prepareInsertOperations: can we extract the bulk of what's common? (the loop seems to be operating on a ColumnDefinition and the associated value, so either pass an Iterable<Pair<...>>, or build 2 lists; I don't think we care about the performance difference here).
            • Nit: getSuperColumnKeyRelation can be private.
            • In prepareDeleteOperations, need to fix the "Maybe not single only" comment. Either IN weren't suppored for this in 2.x and it's enough to make sure it's still refused, or we should support it (note: maybe this is already tested and is something that wan't allowed in 2.x anyway; if so, let's updated or remove the comment).
            • Code style and minor nits:
              • the 2 lines doing if (v instanceof AbstractMarker.Raw) boundNames.add(...); are done enough time that a simple helper might streamline this.
              • In rebuildLWTConditions, I'd invert the initial if check (making it, imo, easier to read) with a direct return and "de-indent" the bulk of the method. Also, I'd remove the columnConditions.clear(): doesn't feel useful, and feels weird to do it here (what if we make that collection immutable in future?).
              • In isSuperColumnMapColumn, can replace the first condition by ColumnDefinition#isRegular (not something due to this patch, but a good occasion to simplify).
              • for 'if-then-else', we never use brakets for one of the branch and not another (meaning that we only skip brackets if all of the branches are one-liner). There is violations to that rule in at least processPartition, getColumnFilter, prepareUpdateOperations and prepareDeleteOperations.
          • In SelectStatement, I believe that if you move the SuperColumnCompatibility#processPartition call at the beginning of SelectStatement#processPartition, you'd be able to remove the specific call in ModificationStatement#buildCasFailureResultSet (and generally reduce the changes of misuses).
          • Slightly bummed by the addition of the SC special fields in StatementRestrictions. Can't we move most of this in SuperColumnCompatibility#getColumnFilter, by extracting things there (tbc, I'm genuninely not sure how easy change that is, and if it's too convoluted, I'm fine leaving things as they are; I just prefer having a much special casing in SuperColumnCompatibility as possible. I also assume we can't entirely remove all special casing from StatementRestrictions since we will at least make sure it doesn't complain about requiring ALLOW FILTERING if we leave the "superColumnKeyColumn" restriciton in)?
          • In ModificationStatement why not directly use cfm.superCfValueColumn instead of looping over cfm.allColumnsInSeelectOrder() to find it?
          Show
          slebresne Sylvain Lebresne added a comment - Thanks for the update and great job on the much needed additional testing. I think we're getting there but I still have a bunch of remarks In CFMetaData In the ctor, for isDense , we seem to force recomputation for non-SC non-dense tables (which happens to include non-compact tables). I don't think thats what we want (and the recalculateIsDense is obviously meant only for SC). But for I'm also not 100% sure this work in all upgrade scenario. Typically, if a dense SCF is upgraded to a current 3.x version, I believe it will end up with 2 regular column definition: the first coming from the fact 2.x will have had a "COMPACT_VALUE" column definition, which will become a REGULAR when upgraded, and second being the "super column map" column that is force-added to all SCF. And so that re-computation code will not detect things properly. Note that I haven't actually tested this tbh so I could be wrong, but this highlight that we really need to be running some SC upgrade dtest both for 2.x -> 3.x+<this patch> and 2.x -> current 3.x -> 3.x+<this patch> because things are imo too complex to rely on reasoning alone. Unfortunately, while we were supposed to have a SC upgrade dtests , it appears this commit has made this test useless by making it (confusingly) not about super column families. So we'll need to revert that commit somehow and the improve the test to include both dense and sparse SC. In rebuild() , why not move the SC code after columnMetadata has been populated so it can be simplified? Nothing seems to rely on it being done before columnMetadata is populated. I'm not entirely sure if/how column renaming works for the SC key and value "fake" column (would have assumed AlterTableStatement would need a least a bit of special casing to recognize those "fake" column). It would be nice to add at test to check that renames still work like in 2.2 for those columns. In UpdateParameters#addCounter , not sure the change from CounterContext#createUpdate to CounterContext#createLocal is corret. Probably a bad post- CASSANDRA-13691 rebase? In {{SuperColumnCompatibility}: The first line of the javadoc states that it's only for "dense" SC, but it's not 100% true as things like SUPER_COLUMN_MAP_COLUMN are used for all SC. And the rest of the javadoc describing the internal layout also applies to all SC. Bit of a detail I suppose, but I'd rephrase things to avoid any confusion. In getSuperCfKeyColumn , not sure what "3.x-created supercolumn family" refers to (is that SC tables created from thrift in 3.x? or SC tables whose schema had been upgraded by a previous 3.x version?) and more importantly, why it would differ in number of clustering columns? I'm confused by the code of getSuperCfValueColumn : dense tables are supposed to have only a single regular column, and cfm.compactValueColumn() is supposed to alias it. So I'm not sure what the loop is trying to do, except basically returning cfm.compactValueColumn() , nor when that loop wouldn't return (all SC should have the "super column map" internally. Further, due to what's above, said loop appears to return a definition whose type is the MapType , while the code following it clearly return a definition whose type is the values of said MapType , so it feels wrong the method would return different kind of types depending on the path taken. I'm most likely missing something since your tests seem to be working, but I'm not sure what. In processPartition : There is a typo in a comment, where in "... can't be change to support inclusive slice ...", I believe you want "exclusive". In the REGULAR branch of the switch : in the 2nd branch ( else if ... ), we should use result.add(cell) directly. Not only does that already handle counters, but more importantly, if we don't do that, things like ttl(v) will not work on the compact column value. the last else is confusing: by definition we shouldn't be exposing any other column than the "compact value" column, so that branch should probably not be there (or be an assertion that we shouldn't get there). In the updates loop in prepareUpdateOperations , not sure why the 2 first cases seem to assume that cfm.isSuperColumnValueColumn(def) , but the 3rd case tests it explicitely (but after having assigned superColumnValue ). In general, not sure why the 3rd case looks different from the other 2. Also, I find it a bit hard to follow the mix of if using continue and sometime using else , sometimes not. I'm personally prefer using else when needed and remove all the continue . Lastly, seems the method could use a check for superColumnKey != null . In prepareInsertForJSONOperations , I believe the 2nd call to getRawTermForColumn is unecessary, you can use the raw for before the if . Further, the overall code of this method feels very similar to the one in prepareInsertOperations : can we extract the bulk of what's common? (the loop seems to be operating on a ColumnDefinition and the associated value, so either pass an Iterable<Pair<...>>, or build 2 lists; I don't think we care about the performance difference here). Nit: getSuperColumnKeyRelation can be private. In prepareDeleteOperations , need to fix the "Maybe not single only" comment. Either IN weren't suppored for this in 2.x and it's enough to make sure it's still refused, or we should support it (note: maybe this is already tested and is something that wan't allowed in 2.x anyway; if so, let's updated or remove the comment). Code style and minor nits: the 2 lines doing if (v instanceof AbstractMarker.Raw) boundNames.add(...); are done enough time that a simple helper might streamline this. In rebuildLWTConditions , I'd invert the initial if check (making it, imo, easier to read) with a direct return and "de-indent" the bulk of the method. Also, I'd remove the columnConditions.clear() : doesn't feel useful, and feels weird to do it here (what if we make that collection immutable in future?). In isSuperColumnMapColumn , can replace the first condition by ColumnDefinition#isRegular (not something due to this patch, but a good occasion to simplify). for 'if-then-else', we never use brakets for one of the branch and not another (meaning that we only skip brackets if all of the branches are one-liner). There is violations to that rule in at least processPartition , getColumnFilter , prepareUpdateOperations and prepareDeleteOperations . In SelectStatement , I believe that if you move the SuperColumnCompatibility#processPartition call at the beginning of SelectStatement#processPartition , you'd be able to remove the specific call in ModificationStatement#buildCasFailureResultSet (and generally reduce the changes of misuses). Slightly bummed by the addition of the SC special fields in StatementRestrictions . Can't we move most of this in SuperColumnCompatibility#getColumnFilter , by extracting things there (tbc, I'm genuninely not sure how easy change that is, and if it's too convoluted, I'm fine leaving things as they are; I just prefer having a much special casing in SuperColumnCompatibility as possible. I also assume we can't entirely remove all special casing from StatementRestrictions since we will at least make sure it doesn't complain about requiring ALLOW FILTERING if we leave the "superColumnKeyColumn" restriciton in)? In ModificationStatement why not directly use cfm.superCfValueColumn instead of looping over cfm.allColumnsInSeelectOrder() to find it?
          Hide
          ifesdjeen Alex Petrov added a comment -

          In the ctor, for isDense, we seem to force re-computation for non-SC non-dense tables (which happens to include non-compact tables).

          Corrected it. This didn't cause any trouble (because was checking for an empty name, too), but now we'll recompute only for the supercolumn ones, which is much better.

          But for I'm also not 100% sure this work in all upgrade scenario.

          Thing is the re-computation is necessary only for the upgrade from 2.x -> current 3.x -> 3.x + this patch, in which case we have two clustering columns instead of one just a single "empty" value column, which is a collection. 3.x-created supercolumns are calculated correctly here.

          You are right that some testing in that regard won't hurt, so I've added some 3-step upgrade tests with the scenarios similar to the current one. For the sakes of completeness, I've also added a test for current 3.x -created supercolumn families.

          Unfortunately, while we were supposed to have a SC upgrade dtests

          I've added some dtests as well as upgrade tests.

          I'm not entirely sure if/how column renaming works for the SC key and value "fake" column

          You're right: I did leave it off as I thought read/write path is the only support that's required for migrating off SCF, but adding this is trivial. I've added tests for all sorts of renames. However, there is no special-casing done in AlterStatement: getColumnDefinition would return the "fake" columns as well. removeColumn would be a no-op in case of fake columns and adding column would force re-initialisation and the new column would get picked up correctly.

          In UpdateParameters#addCounter, not sure the change from CounterContext#createUpdate to CounterContext#createLocal is corret.

          Yes, sorry, I have overlooked it.

          In getSuperCfKeyColumn, not sure what "3.x-created supercolumn family" refers to (is that SC tables created from thrift in 3.x? or SC tables whose schema had been upgraded by a previous 3.x version?) and more importantly, why it would differ in number of clustering columns?

          You are right, SC columns created from Thrift in 3.x. When upgrading from 2.x, you'll get column1 and column2 as clustering keys (which they kind of are). When created via thrift in 3.x, column2 is entirely virtual (which it kind of is). 

          There is a typo in a comment, where in "... can't be change to support inclusive slice ...", I believe you want "exclusive".

          Fixed.

          in the 2nd branch (else if ...), we should use result.add(cell) directly. Not only does that already handle counters, but more importantly, if we don't do that, things like ttl(v) will not work on the compact column value.

          Good point. Fixed.

          the last else is confusing: by definition we shouldn't be exposing any other column than the "compact value" column, so that branch should probably not be there (or be an assertion that we shouldn't get there).

          You're right. This is an artefact of the previous incorrect dense table handling.

          In the updates loop in prepareUpdateOperations, not sure why the 2 first cases seem to assume that cfm.isSuperColumnValueColumn(def), but the 3rd case tests it explicitely (but after having assigned superColumnValue). In general, not sure why the 3rd case looks different from the other 2. Also, I find it a bit hard to follow the mix of if using continue and sometime using else, sometimes not. I'm personally prefer using else when needed and remove all the continue. Lastly, seems the method could use a check for superColumnKey != null.

          I've refactored this method even further. There were many things that were technically correct, but were added on earlier stages, so got there historically. Also, did a bit of further improvements with collectMarkerSpecifications

          In prepareInsertForJSONOperations, I believe the 2nd call to getRawTermForColumn is unecessary, you can use the raw for before the if. Further, the overall code of this method feels very similar to the one in prepareInsertOperations: can we extract the bulk of what's common? (the loop seems to be operating on a ColumnDefinition and the associated value, so either pass an Iterable<Pair<...>>, or build 2 lists; I don't think we care about the performance difference here).

          Refactored this part as well.

          In prepareDeleteOperations, need to fix the "Maybe not single only" comment.

          I think what was meant was "not only SingleColumnRelation", but this is not true: multi column operations are not allowed. After writing more tests, I did discover that multi-column restrictions were not working properly, so I have added support for them. Also, for IN queries.

          n rebuildLWTConditions, I'd invert the initial if check (making it, imo, easier to read) with a direct return and "de-indent" the bulk of the method. Also, I'd remove the columnConditions.clear(): doesn't feel useful, and feels weird to do it here (what if we make that collection immutable in future?).

          Good point, fixed

          In isSuperColumnMapColumn, can replace the first condition by ColumnDefinition#isRegular (not something due to this patch, but a good occasion to simplify).

          Fixed

          In SelectStatement, I believe that if you move the SuperColumnCompatibility#processPartition call at the beginning of SelectStatement#processPartition, you'd be able to remove the specific call in ModificationStatement#buildCasFailureResultSet (and generally reduce the changes of misuses).

          Fixed

          for 'if-then-else', we never use brakets for one of the branch and not another (meaning that we only skip brackets if all of the branches are one-liner).

          I hope I've fixed all the instances.

          Slightly bummed by the addition of the SC special fields in StatementRestrictions. Can't we move most of this in SuperColumnCompatibility#getColumnFilter, by extracting things there (tbc, I'm genuninely not sure how easy change that is, and if it's too convoluted, I'm fine leaving things as they are; I just prefer having a much special casing in SuperColumnCompatibility as possible. I also assume we can't entirely remove all special casing from StatementRestrictions since we will at least make sure it doesn't complain about requiring ALLOW FILTERING if we leave the "superColumnKeyColumn" restriciton in)?

          I agree this did not look pretty. At the time of writing that felt like it made sense. But after the current refactoring, I had to switch most of the things to restrictions anyways, and there are now more different special cases, so I have opted out for a class that would hold all the restriction special-cases. I've added a lot of documentation inline, hope this should suffice.

          In ModificationStatement why not directly use cfm.superCfValueColumn instead of looping over cfm.allColumnsInSeelectOrder() to find it?

          Good point, simplified that one, too.

          A word on the latest changes in the patch: because we have to support all ALTER statements that are supported by 2.x, we got a bit more logic in getSuper(CfKey|Value)Column now. Also, rebuild was changed in order to avoid accidentally dropping the fake columns, since we have to make sure they do get persisted. In order to properly interface 2.x nodes, there are several special-cases in CFMetadata to make sure we compose Selection correctly. Another big change is the way WHERE clauses are handled, pretty much everywhere. Biggest reason for it is that previous way (having public variables in StatementRestrictions was too ad-hoc and many use-cases were missing. Now we can correctly remap multi-column restrictions on SC key (both EQ and slice), slice (and EQ) restrictions on the SC key and IN restriction. All of them are handled somewhat differently and some require filtering on the very last stage. Where possible I've tried to use collection bounds.

          Show
          ifesdjeen Alex Petrov added a comment - In the ctor, for isDense, we seem to force re-computation for non-SC non-dense tables (which happens to include non-compact tables). Corrected it. This didn't cause any trouble (because was checking for an empty name, too), but now we'll recompute only for the supercolumn ones, which is much better. But for I'm also not 100% sure this work in all upgrade scenario. Thing is the re-computation is necessary only for the upgrade from 2.x -> current 3.x -> 3.x + this patch, in which case we have two clustering columns instead of one just a single "empty" value column, which is a collection. 3.x-created supercolumns are calculated correctly here . You are right that some testing in that regard won't hurt, so I've added some 3-step upgrade tests with the scenarios similar to the current one. For the sakes of completeness, I've also added a test for current 3.x -created supercolumn families. Unfortunately, while we were supposed to have a SC upgrade dtests I've added some dtests as well as upgrade tests. I'm not entirely sure if/how column renaming works for the SC key and value "fake" column You're right: I did leave it off as I thought read/write path is the only support that's required for migrating off SCF, but adding this is trivial. I've added tests for all sorts of renames. However, there is no special-casing done in AlterStatement : getColumnDefinition would return the "fake" columns as well. removeColumn would be a no-op in case of fake columns and adding column would force re-initialisation and the new column would get picked up correctly. In UpdateParameters#addCounter, not sure the change from CounterContext#createUpdate to CounterContext#createLocal is corret. Yes, sorry, I have overlooked it. In getSuperCfKeyColumn, not sure what "3.x-created supercolumn family" refers to (is that SC tables created from thrift in 3.x? or SC tables whose schema had been upgraded by a previous 3.x version?) and more importantly, why it would differ in number of clustering columns? You are right, SC columns created from Thrift in 3.x. When upgrading from 2.x, you'll get column1 and column2 as clustering keys (which they kind of are). When created via thrift in 3.x, column2 is entirely virtual (which it kind of is).  There is a typo in a comment, where in "... can't be change to support inclusive slice ...", I believe you want "exclusive". Fixed. in the 2nd branch (else if ...), we should use result.add(cell) directly. Not only does that already handle counters, but more importantly, if we don't do that, things like ttl(v) will not work on the compact column value. Good point. Fixed. the last else is confusing: by definition we shouldn't be exposing any other column than the "compact value" column, so that branch should probably not be there (or be an assertion that we shouldn't get there). You're right. This is an artefact of the previous incorrect dense table handling. In the updates loop in prepareUpdateOperations, not sure why the 2 first cases seem to assume that cfm.isSuperColumnValueColumn(def), but the 3rd case tests it explicitely (but after having assigned superColumnValue). In general, not sure why the 3rd case looks different from the other 2. Also, I find it a bit hard to follow the mix of if using continue and sometime using else, sometimes not. I'm personally prefer using else when needed and remove all the continue. Lastly, seems the method could use a check for superColumnKey != null. I've refactored this method even further. There were many things that were technically correct, but were added on earlier stages, so got there historically. Also, did a bit of further improvements with collectMarkerSpecifications In prepareInsertForJSONOperations, I believe the 2nd call to getRawTermForColumn is unecessary, you can use the raw for before the if. Further, the overall code of this method feels very similar to the one in prepareInsertOperations: can we extract the bulk of what's common? (the loop seems to be operating on a ColumnDefinition and the associated value, so either pass an Iterable<Pair<...>>, or build 2 lists; I don't think we care about the performance difference here). Refactored this part as well. In prepareDeleteOperations, need to fix the "Maybe not single only" comment. I think what was meant was "not only SingleColumnRelation ", but this is not true: multi column operations are not allowed. After writing more tests, I did discover that multi-column restrictions were not working properly, so I have added support for them. Also, for IN queries. n rebuildLWTConditions, I'd invert the initial if check (making it, imo, easier to read) with a direct return and "de-indent" the bulk of the method. Also, I'd remove the columnConditions.clear(): doesn't feel useful, and feels weird to do it here (what if we make that collection immutable in future?). Good point, fixed In isSuperColumnMapColumn, can replace the first condition by ColumnDefinition#isRegular (not something due to this patch, but a good occasion to simplify). Fixed In SelectStatement, I believe that if you move the SuperColumnCompatibility#processPartition call at the beginning of SelectStatement#processPartition, you'd be able to remove the specific call in ModificationStatement#buildCasFailureResultSet (and generally reduce the changes of misuses). Fixed for 'if-then-else', we never use brakets for one of the branch and not another (meaning that we only skip brackets if all of the branches are one-liner). I hope I've fixed all the instances. Slightly bummed by the addition of the SC special fields in StatementRestrictions. Can't we move most of this in SuperColumnCompatibility#getColumnFilter, by extracting things there (tbc, I'm genuninely not sure how easy change that is, and if it's too convoluted, I'm fine leaving things as they are; I just prefer having a much special casing in SuperColumnCompatibility as possible. I also assume we can't entirely remove all special casing from StatementRestrictions since we will at least make sure it doesn't complain about requiring ALLOW FILTERING if we leave the "superColumnKeyColumn" restriciton in)? I agree this did not look pretty. At the time of writing that felt like it made sense. But after the current refactoring, I had to switch most of the things to restrictions anyways, and there are now more different special cases, so I have opted out for a class that would hold all the restriction special-cases. I've added a lot of documentation inline, hope this should suffice. In ModificationStatement why not directly use cfm.superCfValueColumn instead of looping over cfm.allColumnsInSeelectOrder() to find it? Good point, simplified that one, too. A word on the latest changes in the patch: because we have to support all ALTER statements that are supported by 2.x, we got a bit more logic in getSuper(CfKey|Value)Column now. Also, rebuild was changed in order to avoid accidentally dropping the fake columns, since we have to make sure they do get persisted. In order to properly interface 2.x nodes, there are several special-cases in CFMetadata to make sure we compose Selection correctly. Another big change is the way WHERE clauses are handled, pretty much everywhere. Biggest reason for it is that previous way (having public variables in StatementRestrictions was too ad-hoc and many use-cases were missing. Now we can correctly remap multi-column restrictions on SC key (both EQ and slice), slice (and EQ) restrictions on the SC key and IN restriction. All of them are handled somewhat differently and some require filtering on the very last stage. Where possible I've tried to use collection bounds.
          Hide
          slebresne Sylvain Lebresne added a comment -

          The one main remaining things I'm not sure about is that it seems possible to have different schema (meaning, content of schema tables) for what is essentially the same table, depending on how it was created/upgraded. Namely, it appears a dense SCF may have 1 or 2 clustering and may or may not have definitions for the so-called super column "key" and "values" columns.

          This makes it hard, at least to me, to reason about things and have confidence it always work as expected. This also feels error prone in the future. Typically, most code is written expecting that CFMetaData.primaryKeyColumns() would always be equals to CFMetaData.partitionKeyColumns() + CFMetaData.clusteringColumns(), but that's not necessarilly the case here for SCF (and whether it's the case or not really depend more on how the table was created that the table definition). Note that I'm not saying this particular example is a problem today, I believe it's not, but I'm worried about how fragile this feel.

          So my preference would be to force things to be more consistent. What I mean here is that I would make it so that:

          • in the schema tables, every dense SCF table has 2 CLUSTERING (the 1st "true" clustering, and the 2nd standing for the SC "key" column) and 2 REGULAR definition (the SC "map" and the SC "value" column). Note that I think it's important we save the "key" column as a CLUSTERING one: otherwise, if both the "key" and "value" column definions are REGULAR (as I think they can be in the current patch), you can't distinguish which is which later one (and I think that's a current bug of SuperColumnCompatibility.getSuperCfKeyColumn).
          • but at the level of CFMetaData, we extract the "key" and "value" column to their respective field, but otherwise remove them from clusteringColumns and partitionColumns.

          Other than, a bunch of other largely minor issues:

          • In CFMetaData.renameColumn(), we appear to allow renaming every column for any SCF, including non-dense ones. I don't think that was allowed in 2.x (renaming non-PK columns of non-dense SCF through CQL) and I suggest maintaining non supporting it. In fact, I don't think it's entirely safe in some complex case of users still using thrift and doing schema-changes from it.
          • I don't think the change in CFMetaData.makeLegacyDefaultValidator is correct. That said, I don't think the previous code was correct either. If I'm not mistaken, what we should be returning in the SCF case is ((MapType)compactValueColumn().type).valueComparator().
          • In SuperColumnCompatibility.prepareUpdateOperations, after the first loop, I think we should check that superColumnKey != null (and provide a meaningful error message if that's not the case). I believe otherwise we might NPE when handling the {{Operation}}s created.
          • In SuperColumnCompatibility.columnNameGenerator, I'm not sure I fully understand the reason for always excluding "column1" (despite the comment). Not that it's really a big deal.
          • In SuperColumnCompatiblity.SuperColumnRestrictions, regarding the different javadoc:
            • for the class javadoc, since things are tricky, when saying "the default column names are used", I think that's a good place to remind what "column1" and "column2" means, and that both in term of the internal representation, of their CQL exposure, and of the thrift correspondance. Or maybe move such explanation to the SuperColumnCompatibility class javadoc and point to it?
            • for mutliEQRestriction should be ... AND (column1, column2) = ('value1', 1) but it currently uses a >.
            • for keyInRestriction, the "This operation does not have a direct Thrift counterpart" isn't true. And In fact, I'm not sure why we have to fetch everything and filter: can't we just handle this in getColumnFilter by only selecting the map entries we want? Note that the one operation that does not have a Thrift counterpart is mutliSliceRestriction (and, technically, anything operation on strict bounds since Thrift was always inclusive).
            • for keyEQRestriction, I believe "in `getRowFilter`" is supposed to be "in `getColumnFilter`". Using a "{@link}" probably wouldn't hurt either .
            • Nit: there is a few typo in those comments ("prece*e*ding" instead of "preceding", "exlusive", "enitre", "... in this case since, since ...").

          And a few nitpicks:

          • in MultiColumnRelation, both methods have List<ColumnDefinition> receivers = receivers(cfm), but then in the next line, they call receivers(cfm) instead of just reusing receivers.
          • In Relation, I'd extend the error message to something like "Unsupported operation (" + this + ") on super column family".
          • Last else of 2nd loop in SuperColumnCompatibility.prepareUpdateOperations could use brackets
          • In MultiColumnRestriction.SliceRestriction, if my IDE don't fool me, it appears we don't need to make slice public anymore.
          • In StatementRestrictions, a few added imports are not needed (including the NotImplementedException one).
          Show
          slebresne Sylvain Lebresne added a comment - The one main remaining things I'm not sure about is that it seems possible to have different schema (meaning, content of schema tables) for what is essentially the same table, depending on how it was created/upgraded. Namely, it appears a dense SCF may have 1 or 2 clustering and may or may not have definitions for the so-called super column "key" and "values" columns. This makes it hard, at least to me, to reason about things and have confidence it always work as expected. This also feels error prone in the future. Typically, most code is written expecting that CFMetaData.primaryKeyColumns() would always be equals to CFMetaData.partitionKeyColumns() + CFMetaData.clusteringColumns() , but that's not necessarilly the case here for SCF (and whether it's the case or not really depend more on how the table was created that the table definition). Note that I'm not saying this particular example is a problem today, I believe it's not, but I'm worried about how fragile this feel. So my preference would be to force things to be more consistent. What I mean here is that I would make it so that: in the schema tables, every dense SCF table has 2 CLUSTERING (the 1st "true" clustering, and the 2nd standing for the SC "key" column) and 2 REGULAR definition (the SC "map" and the SC "value" column). Note that I think it's important we save the "key" column as a CLUSTERING one: otherwise, if both the "key" and "value" column definions are REGULAR (as I think they can be in the current patch), you can't distinguish which is which later one (and I think that's a current bug of SuperColumnCompatibility.getSuperCfKeyColumn ). but at the level of CFMetaData , we extract the "key" and "value" column to their respective field, but otherwise remove them from clusteringColumns and partitionColumns . Other than, a bunch of other largely minor issues: In CFMetaData.renameColumn() , we appear to allow renaming every column for any SCF, including non-dense ones. I don't think that was allowed in 2.x (renaming non-PK columns of non-dense SCF through CQL) and I suggest maintaining non supporting it. In fact, I don't think it's entirely safe in some complex case of users still using thrift and doing schema-changes from it. I don't think the change in CFMetaData.makeLegacyDefaultValidator is correct. That said, I don't think the previous code was correct either. If I'm not mistaken, what we should be returning in the SCF case is ((MapType)compactValueColumn().type).valueComparator() . In SuperColumnCompatibility.prepareUpdateOperations , after the first loop, I think we should check that superColumnKey != null (and provide a meaningful error message if that's not the case). I believe otherwise we might NPE when handling the {{Operation}}s created. In SuperColumnCompatibility.columnNameGenerator , I'm not sure I fully understand the reason for always excluding "column1" (despite the comment). Not that it's really a big deal. In SuperColumnCompatiblity.SuperColumnRestrictions , regarding the different javadoc: for the class javadoc, since things are tricky, when saying "the default column names are used", I think that's a good place to remind what "column1" and "column2" means, and that both in term of the internal representation, of their CQL exposure, and of the thrift correspondance. Or maybe move such explanation to the SuperColumnCompatibility class javadoc and point to it? for mutliEQRestriction should be ... AND (column1, column2) = ('value1', 1) but it currently uses a > . for keyInRestriction , the "This operation does not have a direct Thrift counterpart" isn't true. And In fact, I'm not sure why we have to fetch everything and filter: can't we just handle this in getColumnFilter by only selecting the map entries we want? Note that the one operation that does not have a Thrift counterpart is mutliSliceRestriction (and, technically, anything operation on strict bounds since Thrift was always inclusive). for keyEQRestriction , I believe "in `getRowFilter`" is supposed to be "in `getColumnFilter`". Using a "{@link}" probably wouldn't hurt either . Nit: there is a few typo in those comments ("prece*e*ding" instead of "preceding", "exlusive", "enitre", "... in this case since, since ..."). And a few nitpicks: in MultiColumnRelation , both methods have List<ColumnDefinition> receivers = receivers(cfm) , but then in the next line, they call receivers(cfm) instead of just reusing receivers . In Relation , I'd extend the error message to something like "Unsupported operation (" + this + ") on super column family" . Last else of 2nd loop in SuperColumnCompatibility.prepareUpdateOperations could use brackets In MultiColumnRestriction.SliceRestriction , if my IDE don't fool me, it appears we don't need to make slice public anymore. In StatementRestrictions , a few added imports are not needed (including the NotImplementedException one).
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          Thank you for the review and comments.

          I agree that having column2 as clustering is better. I've tried to move most of the special-casing to rebuild and SuperColumnCompatibility. I think the patch got a bit cleaner thanks to that.

          In CFMetaData.renameColumn(), we appear to allow renaming every column for any SCF, including non-dense ones. I don't think that was allowed in 2.x (renaming non-PK columns of non-dense SCF through CQL) and I suggest maintaining non supporting it. In fact, I don't think it's entirely safe in some complex case of users still using thrift and doing schema-changes from it.

          Fixed and added corresponding tests to both 3.0 and 2.2

          I don't think the change in CFMetaData.makeLegacyDefaultValidator is correct. That said, I don't think the previous code was correct either. If I'm not mistaken, what we should be returning in the SCF case is ((MapType)compactValueColumn().type).valueComparator().

          I think it can be simplified even further, since isCounter case will work is because of the compactValueColumn (or map value type) and isCompact call seems to be redundant alltogether.

          In SuperColumnCompatibility.prepareUpdateOperations, after the first loop, I think we should check that superColumnKey != null (and provide a meaningful error message if that's not the case). I believe otherwise we might NPE when handling the {{Operation}}s created.

          Fixed and added a couple more negative tests.

          In SuperColumnCompatibility.columnNameGenerator, I'm not sure I fully understand the reason for always excluding "column1" (despite the comment). Not that it's really a big deal.

          This is still a bit of a problem, although just in one case. When upgrade was done through unpatched 3.x, we end up without column2 and value columns. Now, we try renaming column1 to column1_renamed, and, because column2 is still "virtual" (since it was not renamed), we may end up with column2 being called column1 because of the defaults without this line. I'd like to point out that renaming column2 to column2 and value are not allowed even in that case (since all the columns are now in column metadata map).

          for the class javadoc, since things are tricky, when saying "the default column names are used", I think that's a good place to remind what "column1" and "column2" means, and that both in term of the internal representation, of their CQL exposure, and of the thrift correspondance. Or maybe move such explanation to the SuperColumnCompatibility class javadoc and point to it?

          Improved comments in header and for this inner class.

          for mutliEQRestriction should be ... AND (column1, column2) = ('value1', 1) but it currently uses a >.

          Fixed

          for keyInRestriction, the "This operation does not have a direct Thrift counterpart" isn't true. And In fact, I'm not sure why we have to fetch everything and filter: can't we just handle this in getColumnFilter by only selecting the map entries we want? Note that the one operation that does not have a Thrift counterpart is mutliSliceRestriction (and, technically, anything operation on strict bounds since Thrift was always inclusive).

          I was under impression that ColumnFilter.Builder#select allows just a single collection constraint. Thanks for catching that. You're right, we can handle it without any filtering, looks much better now.

          Nit: there is a few typo in those comments ("prece*e*ding" instead of "preceding", "exlusive", "enitre", "... in this case since, since ...").

          Fixed these and spell-checked to catch a couple more.

          in MultiColumnRelation, both methods have List<ColumnDefinition> receivers = receivers(cfm), but then in the next line, they call receivers(cfm) instead of just reusing receivers.

          Fixed.

          In Relation, I'd extend the error message to something like "Unsupported operation (" + this + ") on super column family".

          Fixed.

          Last else of 2nd loop in SuperColumnCompatibility.prepareUpdateOperations could use brackets

          Fixed this an several other ones.

          In MultiColumnRestriction.SliceRestriction, if my IDE don't fool me, it appears we don't need to make slice public anymore.

          You're right. Fixed.

          In StatementRestrictions, a few added imports are not needed (including the NotImplementedException one).

          Fixed this and several other cases.

          3.0 patch 3.11 patch dtests 2.2 patch

          UPDATE: the latest test runs look clean on both branches.

          Show
          ifesdjeen Alex Petrov added a comment - - edited Thank you for the review and comments. I agree that having column2 as clustering is better. I've tried to move most of the special-casing to rebuild and SuperColumnCompatibility . I think the patch got a bit cleaner thanks to that. In CFMetaData.renameColumn(), we appear to allow renaming every column for any SCF, including non-dense ones. I don't think that was allowed in 2.x (renaming non-PK columns of non-dense SCF through CQL) and I suggest maintaining non supporting it. In fact, I don't think it's entirely safe in some complex case of users still using thrift and doing schema-changes from it. Fixed and added corresponding tests to both 3.0 and 2.2 I don't think the change in CFMetaData.makeLegacyDefaultValidator is correct. That said, I don't think the previous code was correct either. If I'm not mistaken, what we should be returning in the SCF case is ((MapType)compactValueColumn().type).valueComparator(). I think it can be simplified even further, since isCounter case will work is because of the compactValueColumn (or map value type) and isCompact call seems to be redundant alltogether. In SuperColumnCompatibility.prepareUpdateOperations, after the first loop, I think we should check that superColumnKey != null (and provide a meaningful error message if that's not the case). I believe otherwise we might NPE when handling the {{Operation}}s created. Fixed and added a couple more negative tests. In SuperColumnCompatibility.columnNameGenerator, I'm not sure I fully understand the reason for always excluding "column1" (despite the comment). Not that it's really a big deal. This is still a bit of a problem, although just in one case. When upgrade was done through unpatched 3.x, we end up without column2 and value columns. Now, we try renaming column1 to column1_renamed , and, because column2 is still "virtual" (since it was not renamed), we may end up with column2 being called column1 because of the defaults without this line. I'd like to point out that renaming column2 to column2 and value are not allowed even in that case (since all the columns are now in column metadata map). for the class javadoc, since things are tricky, when saying "the default column names are used", I think that's a good place to remind what "column1" and "column2" means, and that both in term of the internal representation, of their CQL exposure, and of the thrift correspondance. Or maybe move such explanation to the SuperColumnCompatibility class javadoc and point to it? Improved comments in header and for this inner class. for mutliEQRestriction should be ... AND (column1, column2) = ('value1', 1) but it currently uses a >. Fixed for keyInRestriction, the "This operation does not have a direct Thrift counterpart" isn't true. And In fact, I'm not sure why we have to fetch everything and filter: can't we just handle this in getColumnFilter by only selecting the map entries we want? Note that the one operation that does not have a Thrift counterpart is mutliSliceRestriction (and, technically, anything operation on strict bounds since Thrift was always inclusive). I was under impression that ColumnFilter.Builder#select allows just a single collection constraint. Thanks for catching that. You're right, we can handle it without any filtering, looks much better now. Nit: there is a few typo in those comments ("prece*e*ding" instead of "preceding", "exlusive", "enitre", "... in this case since, since ..."). Fixed these and spell-checked to catch a couple more. in MultiColumnRelation, both methods have List<ColumnDefinition> receivers = receivers(cfm), but then in the next line, they call receivers(cfm) instead of just reusing receivers. Fixed. In Relation, I'd extend the error message to something like "Unsupported operation (" + this + ") on super column family". Fixed. Last else of 2nd loop in SuperColumnCompatibility.prepareUpdateOperations could use brackets Fixed this an several other ones. In MultiColumnRestriction.SliceRestriction, if my IDE don't fool me, it appears we don't need to make slice public anymore. You're right. Fixed. In StatementRestrictions, a few added imports are not needed (including the NotImplementedException one). Fixed this and several other cases. 3.0 patch 3.11 patch dtests 2.2 patch UPDATE: the latest test runs look clean on both branches.
          Hide
          kohlisankalp sankalp kohli added a comment -

          Is this not too late for 3.0?

          Show
          kohlisankalp sankalp kohli added a comment - Is this not too late for 3.0?
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          It's a case of better late than never. It's also a necessary JIRA to allow to move to 4.0.

          Show
          iamaleksey Aleksey Yeschenko added a comment - It's a case of better late than never. It's also a necessary JIRA to allow to move to 4.0.
          Hide
          slebresne Sylvain Lebresne added a comment -

          Thanks for the changes and you patience on this. My main remaining remark is that I don't think we should include the SC key and value columns to partitionColumns (in CFMetaData.rebuild).

          PartitionColumns is meant and used for the columns of the internal storage engine, but the SC key and value columns are "fake" columns used for the CQL translation, they will never have values internally, and so should never reach deep in the storage engine. Which means they shouldn't be in PartitionColumns. In fact, I suspect that's why you needed to have special code in Columns and SerializationHeader, which feels wrong because you shouldn't ever encounter those definitions that deep in the storage engine.

          Don't get me wrong, I'm sure there may be a few places in the CQL layers where we rely on CFMetaData.partitionColumns() and need those columns, and that's probably why you did that, but we imo need to identify those places and special case them.

          Related to this (because due to this), I think the change in ColumnFamilyStoreCQLHelperTest is incorrect: it would be appropriate for ColumnFamilyStoreCQLHelper to either display the storage schema (so no "column2" nor "value"), or the CQL one (so no SCF empty-named map), but something is between is not consistent. Anyway, mainly pointing that we really need to remove those columns from partitionColumns and revert the change in ColumnFamilyStoreCQLHelperTest.

          Other than that, only a few minor remarks:

          • In CFMetaData.renameColumn, in the case of updating the SC key or value column, I believe we should be updating columnMetadata as well since those columns are listed in it, but that doesn't seem to be the case (not sure how important it is, it might be a following call to rebuild fixes that in practice, but since the method doesn't call rebuild itself, probably better to make sure we handle it).
          • In CFMetaData.makeLegacyDefaultValidator, compact tables with counter will now return BytesType instead of CounterColumnType, which is kind of technically incorrect. To be entirely honest, this doesn't matter currently because that method isn't ever called for non-compact tables (and at this point, probably never will), but if we're going to rely on this, I'd rather make it an assertion than returning something somewhat wrong. Personally, I'd just keep the counter special case and move on, as this has nothing to do with this ticket, but if you prefer transforming it to a assert !isCompactTable(), no complain.
          • Nit: in CFMetaData.renameColumn, the comment "SuperColumn tables allow renaming all columns" doesn't match the code entirely anymore.
          • Nit: in CassandraServer.makeColumnFilter, it would be more readable to just cut the method short if metadata.isDense() before the loop, with maybe a comment explaining why it's ok to do so ("Dense table only have dynamic columns").
          • Nit: in SuperColumnCompatibility.getSuperCfKeyColumn, I don't think the "3.x created supercolumn family" comment is accurate anymore since in ThriftConversion you now add the 2nd clustering column (which, in itself, lgtm). It might be we need to preserve that branch in SuperColumnCompatibility.getSuperCfKeyColumn for some upgrade path, and happy to do so, but should update the comment.
          Show
          slebresne Sylvain Lebresne added a comment - Thanks for the changes and you patience on this. My main remaining remark is that I don't think we should include the SC key and value columns to partitionColumns (in CFMetaData.rebuild ). PartitionColumns is meant and used for the columns of the internal storage engine, but the SC key and value columns are "fake" columns used for the CQL translation, they will never have values internally, and so should never reach deep in the storage engine. Which means they shouldn't be in PartitionColumns . In fact, I suspect that's why you needed to have special code in Columns and SerializationHeader , which feels wrong because you shouldn't ever encounter those definitions that deep in the storage engine. Don't get me wrong, I'm sure there may be a few places in the CQL layers where we rely on CFMetaData.partitionColumns() and need those columns, and that's probably why you did that, but we imo need to identify those places and special case them. Related to this (because due to this), I think the change in ColumnFamilyStoreCQLHelperTest is incorrect: it would be appropriate for ColumnFamilyStoreCQLHelper to either display the storage schema (so no "column2" nor "value"), or the CQL one (so no SCF empty-named map), but something is between is not consistent. Anyway, mainly pointing that we really need to remove those columns from partitionColumns and revert the change in ColumnFamilyStoreCQLHelperTest . Other than that, only a few minor remarks: In CFMetaData.renameColumn , in the case of updating the SC key or value column, I believe we should be updating columnMetadata as well since those columns are listed in it, but that doesn't seem to be the case (not sure how important it is, it might be a following call to rebuild fixes that in practice, but since the method doesn't call rebuild itself, probably better to make sure we handle it). In CFMetaData.makeLegacyDefaultValidator , compact tables with counter will now return BytesType instead of CounterColumnType , which is kind of technically incorrect. To be entirely honest, this doesn't matter currently because that method isn't ever called for non-compact tables (and at this point, probably never will), but if we're going to rely on this, I'd rather make it an assertion than returning something somewhat wrong. Personally, I'd just keep the counter special case and move on, as this has nothing to do with this ticket, but if you prefer transforming it to a assert !isCompactTable() , no complain. Nit: in CFMetaData.renameColumn , the comment "SuperColumn tables allow renaming all columns" doesn't match the code entirely anymore. Nit: in CassandraServer.makeColumnFilter , it would be more readable to just cut the method short if metadata.isDense() before the loop, with maybe a comment explaining why it's ok to do so ("Dense table only have dynamic columns"). Nit: in SuperColumnCompatibility.getSuperCfKeyColumn , I don't think the "3.x created supercolumn family" comment is accurate anymore since in ThriftConversion you now add the 2nd clustering column (which, in itself, lgtm). It might be we need to preserve that branch in SuperColumnCompatibility.getSuperCfKeyColumn for some upgrade path, and happy to do so, but should update the comment.
          Hide
          ifesdjeen Alex Petrov added a comment - - edited

          Thanks for the changes and you patience on this. My main remaining remark is that I don't think we should include the SC key and value columns to partitionColumns (in CFMetaData.rebuild).

          This was surprisingly simple to do.

          In CFMetaData.renameColumn, in the case of updating the SC key or value column, I believe we should be updating columnMetadata as well since those columns are listed in it, but that doesn't seem to be the case (not sure how important it is, it might be a following call to rebuild fixes that in practice, but since the method doesn't call rebuild itself, probably better to make sure we handle it).

          I can't see how this can be helpful because of the subsequent rebuild call, but this also doesn't break anything, so I went ahead and changed it.

          In CFMetaData.makeLegacyDefaultValidator, compact tables with counter will now return BytesType instead of CounterColumnType, which is kind of technically incorrect. To be entirely honest, this doesn't matter currently because that method isn't ever called for non-compact tables (and at this point, probably never will), but if we're going to rely on this, I'd rather make it an assertion than returning something somewhat wrong. Personally, I'd just keep the counter special case and move on, as this has nothing to do with this ticket, but if you prefer transforming it to a assert !isCompactTable(), no complain.

          I've added the isCounter back, no strong opinion here, too.

          Nit: in CFMetaData.renameColumn, the comment "SuperColumn tables allow renaming all columns" doesn't match the code entirely anymore.

          Yeah, I was implying dense ones, but I don't think this comment is of much use here anyways.

          Nit: in SuperColumnCompatibility.getSuperCfKeyColumn, I don't think the "3.x created supercolumn family" comment is accurate anymore since in ThriftConversion you now add the 2nd clustering column (which, in itself, lgtm).

          It's still true for pre-12373 3.x thrift-created supercolumn family tables. We've discussed this offline shortly: there was no good way to force the table update to make all the table look completely the same, so this is the only place we still have to special-case. I've added the pre 12373 remark and hope it's clearer now.

          3.0 patch 3.11 patch dtests 2.2 patch

          UPDATE: updated and rebased both branches, CI looks good.

          Show
          ifesdjeen Alex Petrov added a comment - - edited Thanks for the changes and you patience on this. My main remaining remark is that I don't think we should include the SC key and value columns to partitionColumns (in CFMetaData.rebuild). This was surprisingly simple to do. In CFMetaData.renameColumn, in the case of updating the SC key or value column, I believe we should be updating columnMetadata as well since those columns are listed in it, but that doesn't seem to be the case (not sure how important it is, it might be a following call to rebuild fixes that in practice, but since the method doesn't call rebuild itself, probably better to make sure we handle it). I can't see how this can be helpful because of the subsequent rebuild call, but this also doesn't break anything, so I went ahead and changed it. In CFMetaData.makeLegacyDefaultValidator, compact tables with counter will now return BytesType instead of CounterColumnType, which is kind of technically incorrect. To be entirely honest, this doesn't matter currently because that method isn't ever called for non-compact tables (and at this point, probably never will), but if we're going to rely on this, I'd rather make it an assertion than returning something somewhat wrong. Personally, I'd just keep the counter special case and move on, as this has nothing to do with this ticket, but if you prefer transforming it to a assert !isCompactTable(), no complain. I've added the isCounter back, no strong opinion here, too. Nit: in CFMetaData.renameColumn, the comment "SuperColumn tables allow renaming all columns" doesn't match the code entirely anymore. Yeah, I was implying dense ones, but I don't think this comment is of much use here anyways. Nit: in SuperColumnCompatibility.getSuperCfKeyColumn, I don't think the "3.x created supercolumn family" comment is accurate anymore since in ThriftConversion you now add the 2nd clustering column (which, in itself, lgtm). It's still true for pre-12373 3.x thrift-created supercolumn family tables. We've discussed this offline shortly: there was no good way to force the table update to make all the table look completely the same, so this is the only place we still have to special-case. I've added the pre 12373 remark and hope it's clearer now. 3.0 patch 3.11 patch dtests 2.2 patch UPDATE: updated and rebased both branches, CI looks good.
          Hide
          slebresne Sylvain Lebresne added a comment -

          Well, that's a +1 from me. Great job Alex Petrov on this not-too-fun issue.

          Is this not too late for 3.0?

          As Aleksey says, but I'll also add that it's actually fixing a breaking change from 2.x that could prevent some users from updating, so it really should be 3.0.

          Show
          slebresne Sylvain Lebresne added a comment - Well, that's a +1 from me. Great job Alex Petrov on this not-too-fun issue. Is this not too late for 3.0? As Aleksey says, but I'll also add that it's actually fixing a breaking change from 2.x that could prevent some users from updating, so it really should be 3.0.
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          And thanks from me to you both. I had it assigned to me for review for too long and still couldn't find enough energy to do it. Sorry. It really is unfun - both to implement and to review.

          Show
          iamaleksey Aleksey Yeschenko added a comment - And thanks from me to you both. I had it assigned to me for review for too long and still couldn't find enough energy to do it. Sorry. It really is unfun - both to implement and to review.
          Hide
          ifesdjeen Alex Petrov added a comment -

          Thank you for the thorough review & patience Sylvain Lebresne, great job on your side.

          Compatibility tests are committed to 2.2 with c510e001481637e1f74d9ad176f8dc3ab7ebd1e3.

          Patch itself is committed to 3.0 with ce8c9b559f48e72cb4488e75211be338d28bdb13, merged up to 3.11 and trunk (trunk is merged with -s ours as agreed).

          Show
          ifesdjeen Alex Petrov added a comment - Thank you for the thorough review & patience Sylvain Lebresne , great job on your side. Compatibility tests are committed to 2.2 with c510e001481637e1f74d9ad176f8dc3ab7ebd1e3 . Patch itself is committed to 3.0 with ce8c9b559f48e72cb4488e75211be338d28bdb13 , merged up to 3.11 and trunk (trunk is merged with -s ours as agreed).

            People

            • Assignee:
              ifesdjeen Alex Petrov
              Reporter:
              slebresne Sylvain Lebresne
              Reviewer:
              Sylvain Lebresne
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development