You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "smiklosovic (via GitHub)" <gi...@apache.org> on 2025/10/31 12:48:54 UTC

[PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

smiklosovic opened a new pull request, #4458:
URL: https://github.com/apache/cassandra/pull/4458

   Thanks for sending a pull request! Here are some tips if you're new here:
    
    * Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
    * Be sure to keep the PR description updated to reflect all changes.
    * Write your PR title to summarize what this PR proposes.
    * If possible, provide a concise example to reproduce the issue for a faster review.
    * Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
    * If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
    
   Commit messages should follow the following format:
   
   ```
   <One sentence description, usually Jira title or CHANGES.txt summary>
   
   <Optional lengthier description (context on patch)>
   
   patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
   
   Co-authored-by: Name1 <email1>
   Co-authored-by: Name2 <email2>
   
   ```
   
   The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490220356


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   I open to your solution as well though, not sure what is better. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490137872


##########
src/java/org/apache/cassandra/db/compression/ZstdCompressionDictionary.java:
##########
@@ -38,16 +39,28 @@ public class ZstdCompressionDictionary implements CompressionDictionary, SelfRef
 
     private final DictId dictId;
     private final byte[] rawDictionary;
+    private volatile int checksum;

Review Comment:
   It might be changed to final as well. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490094646


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   We can do that but that would make converting logic for composite data etc. more complicated as we would suddenly have two versions of what we want to convert. (CompressionDictionary descendants or custom class as you suggested here). I consider this to be actually the way to go and then we can work with CompressionDictionary later in the code as with any other. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490044105


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   We can do that but how do we differentiate between genuinely empty list and empty list as the result of some error? (as I assume we would return empty list when we catch an exception).
   
   If there is some bug in `CompressionDictionary.createFromRowLightweight`, or in the processing logic as such, and we throw, then we will happily return empty list and we will act as if nothing happened. 
   
   I would at least log in catch before returning empty list, same when returning null from other methods so we have at least some chance to see what went wrong. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2495999305


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -458,22 +458,78 @@ public static CompressionDictionary retrieveLatestCompressionDictionary(String k
     }
 
     /**
-     * Retrieves a specific compression dictionary for a given keyspace and table.
+     * Retrieves the latest compression dictionary for a given keyspace and table
+     * backed by {@link LightweightCompressionDictionary} object.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the latest compression dictionary for the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable
+    public static LightweightCompressionDictionary retrieveLightweightLatestCompressionDictionary(String keyspaceName, String tableName)
+    {
+        String query = "SELECT keyspace_name, table_name, kind, dict_id, dict_checksum, dict_length FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s' LIMIT 1";
+        String fmtQuery = format(query, SchemaConstants.DISTRIBUTED_KEYSPACE_NAME, COMPRESSION_DICTIONARIES, keyspaceName, tableName);
+        try
+        {
+            return CompressionDictionary.createFromRowLightweight(QueryProcessor.execute(fmtQuery, ConsistencyLevel.ONE).one());
+        }
+        catch (Exception e)
+        {
+            return null;
+        }
+    }
+
+    /**
+     * Retrieves a specific compression dictionary for a given keyspace and table
+     * backed by {@link LightweightCompressionDictionary} object.
      *
      * @param keyspaceName the keyspace name to retrieve the dictionary for
      * @param tableName the table name to retrieve the dictionary for
      * @param dictionaryId the dictionary id to retrieve the dictionary for
      * @return the compression dictionary identified by the specified keyspace, table and dictionaryId,
      *         or null if no dictionary exists or if an error occurs during retrieval
      */
-    public static CompressionDictionary retrieveCompressionDictionary(String keyspaceName, String tableName, CompressionDictionary.DictId dictionaryId)
+    @Nullable
+    public static CompressionDictionary retrieveCompressionDictionary(String keyspaceName, String tableName, long dictionaryId)
     {
         String query = "SELECT kind, dict_id, dict, dict_length, dict_checksum FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s' AND dict_id=%s";
-        String fmtQuery = format(query, SchemaConstants.DISTRIBUTED_KEYSPACE_NAME, COMPRESSION_DICTIONARIES, keyspaceName, tableName, dictionaryId.id);
+        String fmtQuery = format(query, SchemaConstants.DISTRIBUTED_KEYSPACE_NAME, COMPRESSION_DICTIONARIES, keyspaceName, tableName, dictionaryId);
         try
         {
-            UntypedResultSet.Row row = QueryProcessor.execute(fmtQuery, ConsistencyLevel.ONE).one();
-            return CompressionDictionary.createFromRow(row);
+            return CompressionDictionary.createFromRow(QueryProcessor.execute(fmtQuery, ConsistencyLevel.ONE).one());
+        }
+        catch (Exception e)
+        {
+            return null;
+        }
+    }
+
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable
+    public static List<LightweightCompressionDictionary> retrieveLightweightCompressionDictionaries(String keyspaceName, String tableName)
+    {
+        String query = "SELECT keyspace_name, table_name, kind, dict_id, dict_length, dict_checksum, size FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s'";
+        String fmtQuery = format(query, SchemaConstants.DISTRIBUTED_KEYSPACE_NAME, COMPRESSION_DICTIONARIES, keyspaceName, tableName);
+        try
+        {
+            List<LightweightCompressionDictionary> dictionaries = new ArrayList<>();
+            UntypedResultSet result = QueryProcessor.execute(fmtQuery, ConsistencyLevel.ONE);

Review Comment:
   nit: could delay the creation of array list.
   ```suggestion
               UntypedResultSet result = QueryProcessor.execute(fmtQuery, ConsistencyLevel.ONE);
               if (result.isEmpty())
                   return Collections.emptyList();
               List<LightweightCompressionDictionary> dictionaries = new ArrayList<>();
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490109485


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)

Review Comment:
   This is the pattern I took from SnapshotDetailsTabularData. If you look into more "TabularData" classes like that, this way of doing it will be most probably there as well.
   
   I dont mind to rename them but we would start to be inconsistent with the rest, heh. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2489814840


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -49,6 +49,20 @@ public interface CompressionDictionary extends AutoCloseable
      */
     byte[] rawDictionary();
 
+    /**
+     * Get checkum of this dictionary.
+     *
+     * @return checksum of this dictionary
+     */
+    int checksum();
+
+    /**
+     * Get size of raw dictionary.
+     *
+     * @return size of raw dictionary, in bytes
+     */
+    int size();

Review Comment:
   because when this is used in lightweight version, rawDictionary() will return null. Then when converting it to CompositeData on dictionary listing, I can't do `rawDictionary().length` on it.
   
   Basically when fetching lightweight version, I can't derive the size of a dictionary when I am not fetching it.
   
   Also check the comment below. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490220356


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   I am open to your solution as well though, not sure what is better. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2517773499


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,304 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularType;
+
+import org.apache.cassandra.db.compression.CompressionDictionary.LightweightCompressionDictionary;
+import org.apache.cassandra.io.util.FileUtils;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    /**
+     * Position inside index names of tabular type of tabular data returned upon
+     * listing dictionaries where raw dictionary is expected to be located.
+     * We do not need to process this entry as listing does not contain any raw dictionary,
+     * only exporting does.
+     */
+    public static final int TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX = 3;
+
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * This method is meant to be call when listing dictionaries, we do not need actual dictionary byte arrays.
+     *
+     * @param dictionary lightweight dictionary to create composite data from
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromLightweightCompressionDictionary(LightweightCompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dictionary.keyspaceName,
+                                            dictionary.tableName,
+                                            dictionary.dictId.id,
+                                            null, // on purpose not returning actual dictionary
+                                            dictionary.dictId.kind.name(),
+                                            dictionary.checksum,
+                                            dictionary.size,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon exporting a dictionary.
+     *
+     * @param keyspace   keyspace of a dictionary
+     * @param table      table of a dictionary
+     * @param dictionary dictionary itself
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromCompressionDictionary(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.rawDictionary().length,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon deserialisation of data to composite data, e.g. upon importing a dictionary from JSON
+     * string to CompositeData before they are sent over JMX to import method.
+     *
+     * @param dataObject data class object to get composite data from
+     * @return composite data representing data class
+     */
+    public static CompositeData fromCompressionDictionaryDataObject(CompressionDictionaryDataObject dataObject)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dataObject.keyspace,
+                                            dataObject.table,
+                                            dataObject.dictId,
+                                            dataObject.dict,
+                                            dataObject.kind,
+                                            dataObject.dictChecksum,
+                                            dataObject.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create data object from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryDataObject#validate()
+     */
+    public static CompressionDictionaryDataObject fromCompositeData(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryDataObject dataObject = new CompressionDictionaryDataObject();
+        dataObject.keyspace = keyspace;
+        dataObject.table = table;
+        dataObject.dictId = dictId;
+        dataObject.dict = dictionaryBytes;
+        dataObject.kind = kind;
+        dataObject.dictChecksum = checksum;
+        dataObject.dictLength = size;
+
+        dataObject.validate();
+
+        return dataObject;
+    }
+
+    public static class CompressionDictionaryDataObject implements Cloneable
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * An object of this class is considered to be valid if:
+         *
+         * <ul>
+         *     <li>keyspace and table are not null</li>
+         *     <li>dict id is lower than 0</li>
+         *     <li>dict is not null nor empty</li>
+         *     <li>dict lenght is less than or equal to 1MiB</li>
+         *     <li>kind is not null and there is such {@link org.apache.cassandra.db.compression.CompressionDictionary.Kind}</li>
+         *     <li>dictLenght is bigger than 0</li>
+         *     <li>dictLenght has to be equal to dict's length</li>
+         *     <li>dictChecksum has to be equal to checksum computed as part of this method</li>
+         * </ul>
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");
+            if (dict.length > FileUtils.ONE_MIB)
+                throw new IllegalArgumentException("Imported dictionary can not be larger than " +
+                                                   FileUtils.ONE_MIB + " bytes, but it is " +
+                                                   dict.length + " bytes.");
+            if (kind == null)
+                throw new IllegalArgumentException("Provided kind is null.");
+
+            CompressionDictionary.Kind dictionaryKind;
+
+            try
+            {
+                dictionaryKind = CompressionDictionary.Kind.valueOf(kind);
+            }
+            catch (IllegalArgumentException ex)
+            {
+                throw new IllegalArgumentException("There is no such dictionary kind like '" + kind + "'. Available kinds: " + Arrays.asList(CompressionDictionary.Kind.values()));
+            }
+
+            if (dictLength <= 0)
+                throw new IllegalArgumentException("Size has to be strictly positive number, it is '" + dictLength + "'.");
+            if (dict.length != dictLength)
+                throw new IllegalArgumentException("The length of the provided dictionary array (" + dict.length + ") is not equal to provided length value (" + dictLength + ").");
+
+            int checksumOfDictionaryToImport = CompressionDictionary.calculateChecksum((byte) dictionaryKind.ordinal(), dictId, dict);
+            if (checksumOfDictionaryToImport != dictChecksum)
+            {
+                throw new IllegalArgumentException(format("Computed checksum of dictionary to import (%s) is different from checksum specified on input (%s).",
+                                                          checksumOfDictionaryToImport,
+                                                          dictChecksum));
+            }
+        }
+
+        @Override
+        public CompressionDictionaryDataObject clone()
+        {
+            try
+            {
+                CompressionDictionaryDataObject clone = (CompressionDictionaryDataObject) super.clone();
+                if (this.dict != null)
+                {
+                    clone.dict = this.dict.clone();
+                }
+                return clone;
+            }
+            catch (CloneNotSupportedException e)
+            {
+                throw new AssertionError("Clone not supported", e);
+            }
+        }
+    }

Review Comment:
   Yes, just for tests ... I think this is simple enough solution for what I want to achieve (have a copy of an object fast) but if you see it not right, I have reworked it to use builder.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2481320229


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -188,9 +220,10 @@ enum Kind
         // Order matters: the enum ordinal is serialized
         ZSTD
         {
-            public CompressionDictionary createDictionary(DictId dictId, byte[] dict)
+            @Override
+            public CompressionDictionary createDictionary(DictId dictId, byte[] dict, int checksum)

Review Comment:
   This seems way more comfortable to do. If a reviewer looks into where checksum is worked with, we have it every time, no reason to not use that so we do not need to compute it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490044105


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   We can do that but how do we differentiate between genuinely empty list and empty list as the result of some error? (as I assume we would return empty list when we catch an exception).
   
   If there is some bug in `CompressionDictionary.createFromRowLightweight`, or in the processing logic as such, and we throw, then we will happily return empty list and we will act as if nothing happened. 
   
   I would at least log in catch before returning empty list (perhaps in a non-spamming manner), same when returning null from other methods so we have at least some chance to see what went wrong. 
   
   Anyway, I think that having it like this here now is just fine :) I feel like logging it would be just too much. We would have tests for catching the errors originating from the processing logic anyway.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490094646


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   We can do that but that would make converting logic for composite data etc. more complicated as we would suddenly have two versions of what we want to convert. (CompressionDictionary descendants when we go to _export_ or custom class as you suggested here when we go to _list_ only). I consider this to be actually the way to go and then we can work with CompressionDictionary later in the code as with any other. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490171694


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   How I see it is that it is either addition of `size()` method on CompressionDictionary and we work with logical CompressionDictionary everywhere in converting logic versus not adding it and then we will need to do a lot of other stuff (custom class, another dedicated method in converting to CompositeData).
   
   I just find the current approach more friendly and congruent with how API / interface is used (minus returning null from rawDictionary method we can solve easily by returning something not null (e.g. empty byte array), but that is just an implementation detail). 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2517785286


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -267,6 +336,11 @@ private void handleNewDictionary(CompressionDictionary dictionary)
         onNewDictionaryTrained(dictionary.dictId());
     }
 
+    public void importDictionary(CompressionDictionary dictionary)

Review Comment:
   yes I have noticed this too, resolved.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2489814840


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -49,6 +49,20 @@ public interface CompressionDictionary extends AutoCloseable
      */
     byte[] rawDictionary();
 
+    /**
+     * Get checkum of this dictionary.
+     *
+     * @return checksum of this dictionary
+     */
+    int checksum();
+
+    /**
+     * Get size of raw dictionary.
+     *
+     * @return size of raw dictionary, in bytes
+     */
+    int size();

Review Comment:
   because when this is used in lightweight version, rawDictionary() will return null. Then when converting it to CompositeData on dictionary listing, I can't do `rawDictionary().length` on it.
   
   Basically when fetching lightweight version, I can't derive the size of a dictionary when I am not fetching it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490064610


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary
+    {
+        private final DictId dictId;
+        private final int checksum;
+        private final int size;
+
+        public LightweightCompressionDictionary(DictId dictId, int checksum, int size)
+        {
+            this.dictId = dictId;
+            this.checksum = checksum;
+            this.size = size;
+        }
+
+        @Override
+        public DictId dictId()
+        {
+            return dictId;
+        }
+
+        @Override
+        public byte[] rawDictionary()
+        {
+            return null;

Review Comment:
   null is valid value to return when we just list dictionaries as no actual data will be fetched from the database
   
   We can return empty byte array for the sake of not returning null here and then in the composite / converting logic on listing I would return null if the value returned here is empty array (or I can just return empty byte array to caller, actually).
   
   Throwing an exception is not a good idea because it would complicate the composite logic more unnecessarily.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490064610


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary
+    {
+        private final DictId dictId;
+        private final int checksum;
+        private final int size;
+
+        public LightweightCompressionDictionary(DictId dictId, int checksum, int size)
+        {
+            this.dictId = dictId;
+            this.checksum = checksum;
+            this.size = size;
+        }
+
+        @Override
+        public DictId dictId()
+        {
+            return dictId;
+        }
+
+        @Override
+        public byte[] rawDictionary()
+        {
+            return null;

Review Comment:
   null is valid value to return when we just list dictionaries as no actual data will be fetched from the database, we can return empty byte array for the sake of not returning null here and then in the composite / converting logic on listing I would return null if the value returned here is empty array (or I can just return empty byte array to caller, actually).
   
   Throwing an exception is not a good idea because it would complicate the composite logic more unnecessarily.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2492053322


##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -18,26 +18,44 @@
 package org.apache.cassandra.tools.nodetool;
 
 import java.io.PrintStream;
+import java.util.List;
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
 
 import com.google.common.util.concurrent.Uninterruptibles;
 
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryPojo;
 import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
 import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
 import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
 @Command(name = "compressiondictionary",
-         description = "Manage compression dictionaries",
-         subcommands = { CompressionDictionary.Train.class })
+description = "Manage compression dictionaries",
+subcommands = { CompressionDictionary.TrainDictionary.class,
+                CompressionDictionary.ListDictionaries.class,
+                CompressionDictionary.ExportDictionary.class,
+                CompressionDictionary.ImportDictionary.class })
 public class CompressionDictionary

Review Comment:
   Make sense. I also find the same named classes a bit inconvenient.  



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #4458: CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary
URL: https://github.com/apache/cassandra/pull/4458


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2517802847


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -244,6 +251,68 @@ public CompositeData getTrainingState()
         return dictionaryTrainer.getTrainingState().toCompositeData();
     }
 
+    @Override
+    public TabularData listCompressionDictionaries()
+    {
+        List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspaceName, tableName);
+        TabularDataSupport tableData = new TabularDataSupport(CompressionDictionaryDetailsTabularData.TABULAR_TYPE);
+
+        if (dictionaries == null)
+        {
+            return tableData;
+        }
+
+        for (LightweightCompressionDictionary dictionary : dictionaries)
+        {
+            tableData.put(CompressionDictionaryDetailsTabularData.fromLightweightCompressionDictionary(dictionary));
+        }
+
+        return tableData;
+    }
+
+    @Override
+    public CompositeData getCompressionDictionary()
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveLatestCompressionDictionary(keyspaceName, tableName);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.fromCompressionDictionary(keyspaceName, tableName, compressionDictionary);
+    }
+
+    @Override
+    public CompositeData getCompressionDictionary(long dictId)
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveCompressionDictionary(keyspaceName, tableName, dictId);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.fromCompressionDictionary(keyspaceName, tableName, compressionDictionary);
+    }
+
+    @Override
+    public synchronized void importCompressionDictionary(CompositeData compositeData)
+    {
+        CompressionDictionaryDataObject dataObject = CompressionDictionaryDetailsTabularData.fromCompositeData(compositeData);
+
+        if (!keyspaceName.equals(dataObject.keyspace) || !tableName.equals(dataObject.table))
+            throw new IllegalArgumentException(format("Keyspace and table of a dictionary to import (%s.%s) does not correspond to the keyspace and table this manager is responsible for (%s.%s)",
+                                                      dataObject.keyspace, dataObject.table,
+                                                      keyspaceName, tableName));
+
+        CompressionDictionary.Kind kind = CompressionDictionary.Kind.valueOf(dataObject.kind);
+        CompressionDictionary.DictId dictId = new CompressionDictionary.DictId(kind, dataObject.dictId);
+
+        LightweightCompressionDictionary latestCompressionDictionary = SystemDistributedKeyspace.retrieveLightweightLatestCompressionDictionary(keyspaceName, tableName);

Review Comment:
   What we can do though is to check that _kind_ of the dictionary which is meant to be imported matches the kind of the dictionary manager. It might happen that we would try to import `Kind.MY_DICT_ALGORITHM` while manager would be configured with `Kind.ZSTD` so it would not make any sense to import that.
   
   I think you basically meant this, correct?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2489861562


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,
+         * kind corresponds to {@code Kind}, checksum and size are bigger than 0.
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");

Review Comment:
   1MiB is reasonable limit indeed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490131891


##########
src/java/org/apache/cassandra/db/compression/ZstdCompressionDictionary.java:
##########
@@ -38,16 +39,28 @@ public class ZstdCompressionDictionary implements CompressionDictionary, SelfRef
 
     private final DictId dictId;
     private final byte[] rawDictionary;
+    private volatile int checksum;

Review Comment:
   yes, that's leftover from times I was trying to compute checksum on the fly.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490171694


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   How I see it is that it is either addition of `size()` method on CompressionDictionary and we work with logical CompressionDictionary everywhere in converting logic versus not adding it and then we will need to do a lot of other stuff (custom class, another dedicated method in converting to CompositeData).
   
   I just find the current approach more friendly. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490171694


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   But you are probably right, I think returning something simple is probably better. I will try to model that in followup commits. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490146813


##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -18,25 +18,42 @@
 package org.apache.cassandra.tools.nodetool;
 
 import java.io.PrintStream;
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
 
 import com.google.common.util.concurrent.Uninterruptibles;
 
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryPojo;
 import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
 import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
 import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
 @Command(name = "compressiondictionary",
-         description = "Manage compression dictionaries",
-         subcommands = { CompressionDictionary.Train.class })
+description = "Manage compression dictionaries",
+subcommands = { CompressionDictionary.Train.class,
+                CompressionDictionary.List.class,

Review Comment:
   I renamed this to ListDictionaries but then the rest of commands was just "Export, Import, Train" which is not consistent so I added "Dictionary" suffixes to them as well.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2489829637


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,

Review Comment:
   I remove whole comemnt for now, will add that when dust settles. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2487296008


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -49,6 +49,20 @@ public interface CompressionDictionary extends AutoCloseable
      */
     byte[] rawDictionary();
 
+    /**
+     * Get checkum of this dictionary.

Review Comment:
   typo:
   
   ```suggestion
        * Get checksum of this dictionary.
   ```



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary
+    {
+        private final DictId dictId;
+        private final int checksum;
+        private final int size;
+
+        public LightweightCompressionDictionary(DictId dictId, int checksum, int size)
+        {
+            this.dictId = dictId;
+            this.checksum = checksum;
+            this.size = size;
+        }
+
+        @Override
+        public DictId dictId()
+        {
+            return dictId;
+        }
+
+        @Override
+        public byte[] rawDictionary()
+        {
+            return null;

Review Comment:
   In my opinion, returning null loosen the API contract by permitting `null` return value. Caller has to consider the null case. How about throwing `UnsupportedOperationException` instead? So the API remains return non-null values only. 



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,

Review Comment:
   not sure what does "dictionary byte array is not nor not empty" mean.



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -244,6 +251,68 @@ public CompositeData getTrainingState()
         return dictionaryTrainer.getTrainingState().toCompositeData();
     }
 
+    @Override
+    public TabularData listDictionaries(String keyspace, String table)
+    {
+        List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspace, table);
+        TabularDataSupport tableData = new TabularDataSupport(CompressionDictionaryDetailsTabularData.TABULAR_TYPE);
+
+        if (dictionaries == null)
+        {
+            return tableData;
+        }
+
+        for (LightweightCompressionDictionary dictionary : dictionaries)
+        {
+            CompressionDictionaryDetailsTabularData.from(keyspace, table, dictionary, tableData);
+        }
+
+        return tableData;
+    }
+
+    @Override
+    public CompositeData getDictionary(String keyspace, String table)
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveLatestCompressionDictionary(keyspaceName, tableName);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.from(keyspace, table, compressionDictionary);
+    }
+
+    @Override
+    public CompositeData getDictionary(String keyspace, String table, long dictId)
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveCompressionDictionary(keyspaceName, tableName, dictId);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.from(keyspace, table, compressionDictionary);
+    }
+
+    @Override
+    public void importDictionary(CompositeData compositeData)

Review Comment:
   Should this method be `synchornized`? We would like to avoid importing concurrently. 



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,
+         * kind corresponds to {@code Kind}, checksum and size are bigger than 0.
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");
+            if (kind == null)
+                throw new IllegalArgumentException("Provided kind is null.");
+
+            CompressionDictionary.Kind dictionarykind;

Review Comment:
   nit: snake-case
   ```suggestion
               CompressionDictionary.Kind dictionaryKind;
   ```



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,
+         * kind corresponds to {@code Kind}, checksum and size are bigger than 0.
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");
+            if (kind == null)
+                throw new IllegalArgumentException("Provided kind is null.");
+
+            CompressionDictionary.Kind dictionarykind;
+
+            try
+            {
+                dictionarykind = CompressionDictionary.Kind.valueOf(kind);
+            }
+            catch (IllegalArgumentException ex)
+            {
+                throw new IllegalArgumentException("There is no such dictionary kind like '" + kind + "'. Available kinds: " + Arrays.asList(CompressionDictionary.Kind.values()));
+            }
+
+            if (dictChecksum <= 0)
+                throw new IllegalArgumentException("Checksum has to be strictly positive number, it is '" + dictChecksum + "'.");

Review Comment:
   I think the checksum could be negative. The checksum is from crc32, which produce 32 bits. In signed integer, half of the values are negative. 



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManagerMBean.java:
##########
@@ -44,4 +46,42 @@ public interface CompressionDictionaryManagerMBean
      * @return CompositeData representing {@link TrainingState}
      */
     CompositeData getTrainingState();
+
+    /**
+     * Lists dictionaries for given keyspace and table. Returned dictionaries
+     * do not contain raw dictionary bytes.
+     *
+     * @param keyspace keyspace to get dictionaries from
+     * @param table    table to get dictionaries from
+     * @return dictionaries for given keyspace and table
+     */
+    TabularData listDictionaries(String keyspace, String table);
+
+    /**
+     * Get latest compression dictionary.
+     *
+     * @param keyspace keyspace to get dictionary for
+     * @param table    table to get dictionary for
+     * @return CompositeData representing dictionary or null of not found

Review Comment:
   typo:
   ```suggestion
        * @return CompositeData representing dictionary or null if not found
   ```



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -49,6 +49,20 @@ public interface CompressionDictionary extends AutoCloseable
      */
     byte[] rawDictionary();
 
+    /**
+     * Get checkum of this dictionary.
+     *
+     * @return checksum of this dictionary
+     */
+    int checksum();
+
+    /**
+     * Get size of raw dictionary.
+     *
+     * @return size of raw dictionary, in bytes
+     */
+    int size();

Review Comment:
   Why is this method needed in addition to `byte[] rawDictionary();`?



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   Giving it another thought, should it implement `CompressionDictionary`? What about just have a data class that contains 3 fields? I can see that `LightweightCompressionDictionary` type is used specifically, e.g. `SystemDistributedKeyspace#retrieveLightweightLatestCompressionDictionary` returns `LightweightCompressionDictionary`, not `CompressionDictionary`.
   By having a specific data type would give better type safety and caller may not need to worry about the unnecessary methods (that are marked unsupported or returning null, etc.)



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,

Review Comment:
   typo:
   ```suggestion
        * or the instantiation of a proper dictionary object is not desirable or unnecessary for other,
   ```



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManagerMBean.java:
##########
@@ -44,4 +46,42 @@ public interface CompressionDictionaryManagerMBean
      * @return CompositeData representing {@link TrainingState}
      */
     CompositeData getTrainingState();
+
+    /**
+     * Lists dictionaries for given keyspace and table. Returned dictionaries
+     * do not contain raw dictionary bytes.
+     *
+     * @param keyspace keyspace to get dictionaries from
+     * @param table    table to get dictionaries from
+     * @return dictionaries for given keyspace and table
+     */
+    TabularData listDictionaries(String keyspace, String table);

Review Comment:
   1. Please name the method (this and the other new ones) as `listCompressionDictionaries` or simply `list` if you consider dictionaries is implied already implied by the mbean name. I intentionally stayed about from using "dictionary" alone in the initial patch, because the word "dictionary" is too general. 
   2. I do not think you need to supply `String keyspace, String table`. The MBean, like CFS MBean, is per table. The keyspace and table names are already known. 



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,
+         * kind corresponds to {@code Kind}, checksum and size are bigger than 0.
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");
+            if (kind == null)
+                throw new IllegalArgumentException("Provided kind is null.");
+
+            CompressionDictionary.Kind dictionarykind;
+
+            try
+            {
+                dictionarykind = CompressionDictionary.Kind.valueOf(kind);
+            }
+            catch (IllegalArgumentException ex)
+            {
+                throw new IllegalArgumentException("There is no such dictionary kind like '" + kind + "'. Available kinds: " + Arrays.asList(CompressionDictionary.Kind.values()));
+            }
+
+            if (dictChecksum <= 0)
+                throw new IllegalArgumentException("Checksum has to be strictly positive number, it is '" + dictChecksum + "'.");
+            if (dictLength <= 0)
+                throw new IllegalArgumentException("Size has to be strictly positive number, it is '" + dictLength + "'.");
+            if (dict.length != dictLength)
+                throw new IllegalArgumentException("The length of the provided dictionary array (" + dict.length + ") does not equal to provided lenght value (" + dictLength + ").");

Review Comment:
   typo:
   ```suggestion
                   throw new IllegalArgumentException("The length of the provided dictionary array (" + dict.length + ") does not equal to provided length value (" + dictLength + ").");
   ```



##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -458,18 +459,45 @@ public static CompressionDictionary retrieveLatestCompressionDictionary(String k
     }
 
     /**
-     * Retrieves a specific compression dictionary for a given keyspace and table.
+     * Retrieves the latest compression dictionary for a given keyspace and table
+     * backed by {@link LightweightCompressionDictionary} object.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the latest compression dictionary for the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   👍 on adding the annotation.



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -244,6 +251,68 @@ public CompositeData getTrainingState()
         return dictionaryTrainer.getTrainingState().toCompositeData();
     }
 
+    @Override
+    public TabularData listDictionaries(String keyspace, String table)
+    {
+        List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspace, table);

Review Comment:
   We should use the field variables of the enclosing `CompressionDictionaryManager` instance. 
   ```
       private final String keyspaceName;
       private final String tableName;
   ```



##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -119,4 +136,168 @@ private static void displayProgress(TrainingState trainingState, long startTime,
                        status, sampleCount, sampleSizeMB, elapsedSeconds);
         }
     }
+
+    @Command(name = "list", description = "List available dictionaries of specific keyspace and table.")
+    public static class List extends AbstractCommand

Review Comment:
   nit: Could we rename the command, so that the full qualified name `java.util.List` can be avoided?



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManagerMBean.java:
##########
@@ -44,4 +46,42 @@ public interface CompressionDictionaryManagerMBean
      * @return CompositeData representing {@link TrainingState}
      */
     CompositeData getTrainingState();
+
+    /**
+     * Lists dictionaries for given keyspace and table. Returned dictionaries
+     * do not contain raw dictionary bytes.
+     *
+     * @param keyspace keyspace to get dictionaries from
+     * @param table    table to get dictionaries from
+     * @return dictionaries for given keyspace and table
+     */
+    TabularData listDictionaries(String keyspace, String table);
+
+    /**
+     * Get latest compression dictionary.
+     *
+     * @param keyspace keyspace to get dictionary for
+     * @param table    table to get dictionary for
+     * @return CompositeData representing dictionary or null of not found
+     */
+    @Nullable
+    CompositeData getDictionary(String keyspace, String table);
+
+    /**
+     * Get compression dictionary.
+     *
+     * @param keyspace keyspace to get dictionary for
+     * @param table    table to get dictionary for
+     * @param dictId   id of dictionary to get
+     * @return CompositeData representing dictionary or null of not found
+     */
+    @Nullable
+    CompositeData getDictionary(String keyspace, String table, long dictId);

Review Comment:
   `String keyspace, String table` are redundant.



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManagerMBean.java:
##########
@@ -44,4 +46,42 @@ public interface CompressionDictionaryManagerMBean
      * @return CompositeData representing {@link TrainingState}
      */
     CompositeData getTrainingState();
+
+    /**
+     * Lists dictionaries for given keyspace and table. Returned dictionaries
+     * do not contain raw dictionary bytes.
+     *
+     * @param keyspace keyspace to get dictionaries from
+     * @param table    table to get dictionaries from
+     * @return dictionaries for given keyspace and table
+     */
+    TabularData listDictionaries(String keyspace, String table);
+
+    /**
+     * Get latest compression dictionary.
+     *
+     * @param keyspace keyspace to get dictionary for
+     * @param table    table to get dictionary for
+     * @return CompositeData representing dictionary or null of not found
+     */
+    @Nullable
+    CompositeData getDictionary(String keyspace, String table);

Review Comment:
   `String keyspace, String table` are redundant. 



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,
+         * kind corresponds to {@code Kind}, checksum and size are bigger than 0.
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");

Review Comment:
   Can we add a new check to ensure the importing dictionary size does not exceed certain amount? We will not want to import dictionary that is larger than 1MiB, most likely the dictionary should be less than 100KiB. 



##########
src/java/org/apache/cassandra/db/compression/ZstdCompressionDictionary.java:
##########
@@ -38,16 +39,28 @@ public class ZstdCompressionDictionary implements CompressionDictionary, SelfRef
 
     private final DictId dictId;
     private final byte[] rawDictionary;
+    private volatile int checksum;

Review Comment:
   why is it `volatile`? the value is never updated. 



##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   Should it return an empty list? It is cheap and simplifies the call-site handling. 



##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -18,25 +18,42 @@
 package org.apache.cassandra.tools.nodetool;
 
 import java.io.PrintStream;
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
 
 import com.google.common.util.concurrent.Uninterruptibles;
 
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryPojo;
 import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
 import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
 import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
 @Command(name = "compressiondictionary",
-         description = "Manage compression dictionaries",
-         subcommands = { CompressionDictionary.Train.class })
+description = "Manage compression dictionaries",
+subcommands = { CompressionDictionary.Train.class,
+                CompressionDictionary.List.class,
+                CompressionDictionary.Export.class,
+                CompressionDictionary.Import.class })
 public class CompressionDictionary
 {
     @Command(name = "train",
-             description = "Manually trigger compression dictionary training for a table. If no SSTables are available, the memtable will be flushed first.")
+    description = "Manually trigger compression dictionary training for a table. If no SSTables are available, the memtable will be flushed first.")

Review Comment:
   nit: keep the alignment of the annotation elements? 



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,
+         * kind corresponds to {@code Kind}, checksum and size are bigger than 0.
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");
+            if (kind == null)
+                throw new IllegalArgumentException("Provided kind is null.");
+
+            CompressionDictionary.Kind dictionarykind;
+
+            try
+            {
+                dictionarykind = CompressionDictionary.Kind.valueOf(kind);
+            }
+            catch (IllegalArgumentException ex)
+            {
+                throw new IllegalArgumentException("There is no such dictionary kind like '" + kind + "'. Available kinds: " + Arrays.asList(CompressionDictionary.Kind.values()));
+            }
+
+            if (dictChecksum <= 0)
+                throw new IllegalArgumentException("Checksum has to be strictly positive number, it is '" + dictChecksum + "'.");
+            if (dictLength <= 0)
+                throw new IllegalArgumentException("Size has to be strictly positive number, it is '" + dictLength + "'.");
+            if (dict.length != dictLength)
+                throw new IllegalArgumentException("The length of the provided dictionary array (" + dict.length + ") does not equal to provided lenght value (" + dictLength + ").");
+
+            int checksumOfDictionaryToImport = CompressionDictionary.calculateChecksum((byte) dictionarykind.ordinal(), dictId, dict);
+            if (checksumOfDictionaryToImport != dictChecksum)
+            {
+                throw new IllegalArgumentException(format("Computed checksum of dictionary to import (%s) is different from checksum specified on input (%s).",

Review Comment:
   nit: error message revision. `Compression dictionary checksum mismatched. Actual: %s; provided: %s."



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -244,6 +251,68 @@ public CompositeData getTrainingState()
         return dictionaryTrainer.getTrainingState().toCompositeData();
     }
 
+    @Override
+    public TabularData listDictionaries(String keyspace, String table)
+    {
+        List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspace, table);
+        TabularDataSupport tableData = new TabularDataSupport(CompressionDictionaryDetailsTabularData.TABULAR_TYPE);
+
+        if (dictionaries == null)
+        {
+            return tableData;
+        }
+
+        for (LightweightCompressionDictionary dictionary : dictionaries)
+        {
+            CompressionDictionaryDetailsTabularData.from(keyspace, table, dictionary, tableData);

Review Comment:
   It looks a bit odd that no return value is there in the `from` method and not used. 
   Could you use this instead and delete the unused `from` method? 
   ```java
           for (LightweightCompressionDictionary dictionary : dictionaries)
           {
               tableData.put(CompressionDictionaryDetailsTabularData.from(keyspace, table, dictionary));
           }
   ```
   
   It leads to the question: should the `CompressionDictionaryDetailsTabularData` be called `TabularData`? It is to return the individual compression dictionary details. Would it make better sense to rename to `CompressionDictionaryDetails`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2496046776


##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionaryCommandGroup.java:
##########
@@ -0,0 +1,307 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.tools.nodetool;
+
+import java.io.PrintStream;
+import java.util.List;
+import java.util.ArrayList;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
+
+import com.google.common.util.concurrent.Uninterruptibles;
+
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryDataObject;
+import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
+import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+import picocli.CommandLine.Parameters;
+
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
+@Command(name = "compressiondictionary",
+         description = "Manage compression dictionaries",
+         subcommands = { CompressionDictionaryCommandGroup.TrainDictionary.class,
+                         CompressionDictionaryCommandGroup.ListDictionaries.class,
+                         CompressionDictionaryCommandGroup.ExportDictionary.class,
+                         CompressionDictionaryCommandGroup.ImportDictionary.class })
+public class CompressionDictionaryCommandGroup
+{
+    @Command(name = "train",
+             description = "Manually trigger compression dictionary training for a table. If no SSTables are available, the memtable will be flushed first.")
+    public static class TrainDictionary extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Option(names = { "-f", "--force" }, description = "Force the dictionary training even if there are not enough samples")
+        private boolean force = false;
+
+        @Override
+        public void execute(NodeProbe probe)
+        {
+            PrintStream out = probe.output().out;
+            PrintStream err = probe.output().err;
+
+            try
+            {
+                out.printf("Starting compression dictionary training for %s.%s...%n", keyspace, table);
+                out.printf("Training from existing SSTables (flushing first if needed)%n");
+
+                probe.trainCompressionDictionary(keyspace, table, force);
+
+                // Wait for training completion (10 minutes timeout for SSTable-based training)
+                out.println("Sampling from existing SSTables and training.");
+                long maxWaitMillis = TimeUnit.MINUTES.toMillis(10);
+                long startTime = Clock.Global.currentTimeMillis();
+
+                while (Clock.Global.currentTimeMillis() - startTime < maxWaitMillis)
+                {
+                    TrainingState trainingState = probe.getCompressionDictionaryTrainingState(keyspace, table);
+                    TrainingStatus status = trainingState.getStatus();
+                    displayProgress(trainingState, startTime, out, status);
+                    if (TrainingStatus.COMPLETED == status)
+                    {
+                        out.printf("%nTraining completed successfully for %s.%s%n", keyspace, table);
+                        return;
+                    }
+                    else if (TrainingStatus.FAILED == status)
+                    {
+                        err.printf("%nTraining failed for %s.%s%n", keyspace, table);
+                        try
+                        {
+                            String failureMessage = trainingState.getFailureMessage();
+                            if (failureMessage != null && !failureMessage.isEmpty())
+                            {
+                                err.printf("Reason: %s%n", failureMessage);
+                            }
+                        }
+                        catch (Exception e)
+                        {
+                            // If we can't get the failure message, just continue without it
+                        }
+                        System.exit(1);
+                    }
+
+                    Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+                }
+
+                err.printf("%nTraining did not complete within expected timeframe (10 minutes).%n");
+                System.exit(1);
+            }
+            catch (Exception e)
+            {
+                err.printf("Failed to trigger training: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+
+        private static void displayProgress(TrainingState trainingState, long startTime, PrintStream out, TrainingStatus status)
+        {
+            // Display meaningful statistics
+            long sampleCount = trainingState.getSampleCount();
+            long totalSampleSize = trainingState.getTotalSampleSize();
+            long elapsedSeconds = (Clock.Global.currentTimeMillis() - startTime) / 1000;
+            double sampleSizeMB = totalSampleSize / (1024.0 * 1024.0);
+
+            out.printf("\rStatus: %s | Samples: %d | Size: %.2f MiB | Elapsed: %ds",
+                       status, sampleCount, sampleSizeMB, elapsedSeconds);
+        }
+    }
+
+    @Command(name = "list",
+             description = "List available dictionaries of specific keyspace and table.")
+    public static class ListDictionaries extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Override
+        protected void execute(NodeProbe probe)
+        {
+            try
+            {
+                TableBuilder tableBuilder = new TableBuilder();
+                TabularData tabularData = probe.listCompressionDictionaries(keyspace, table);
+                List<String> indexNames = tabularData.getTabularType().getIndexNames();
+
+                List<String> columns = new ArrayList<>(indexNames);
+                columns.remove(3);  // ignore raw dict

Review Comment:
   nit: how about define a constant for the index and ideally it is defined in `CompressionDictionaryDetailsTabularData`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2517802847


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -244,6 +251,68 @@ public CompositeData getTrainingState()
         return dictionaryTrainer.getTrainingState().toCompositeData();
     }
 
+    @Override
+    public TabularData listCompressionDictionaries()
+    {
+        List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspaceName, tableName);
+        TabularDataSupport tableData = new TabularDataSupport(CompressionDictionaryDetailsTabularData.TABULAR_TYPE);
+
+        if (dictionaries == null)
+        {
+            return tableData;
+        }
+
+        for (LightweightCompressionDictionary dictionary : dictionaries)
+        {
+            tableData.put(CompressionDictionaryDetailsTabularData.fromLightweightCompressionDictionary(dictionary));
+        }
+
+        return tableData;
+    }
+
+    @Override
+    public CompositeData getCompressionDictionary()
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveLatestCompressionDictionary(keyspaceName, tableName);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.fromCompressionDictionary(keyspaceName, tableName, compressionDictionary);
+    }
+
+    @Override
+    public CompositeData getCompressionDictionary(long dictId)
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveCompressionDictionary(keyspaceName, tableName, dictId);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.fromCompressionDictionary(keyspaceName, tableName, compressionDictionary);
+    }
+
+    @Override
+    public synchronized void importCompressionDictionary(CompositeData compositeData)
+    {
+        CompressionDictionaryDataObject dataObject = CompressionDictionaryDetailsTabularData.fromCompositeData(compositeData);
+
+        if (!keyspaceName.equals(dataObject.keyspace) || !tableName.equals(dataObject.table))
+            throw new IllegalArgumentException(format("Keyspace and table of a dictionary to import (%s.%s) does not correspond to the keyspace and table this manager is responsible for (%s.%s)",
+                                                      dataObject.keyspace, dataObject.table,
+                                                      keyspaceName, tableName));
+
+        CompressionDictionary.Kind kind = CompressionDictionary.Kind.valueOf(dataObject.kind);
+        CompressionDictionary.DictId dictId = new CompressionDictionary.DictId(kind, dataObject.dictId);
+
+        LightweightCompressionDictionary latestCompressionDictionary = SystemDistributedKeyspace.retrieveLightweightLatestCompressionDictionary(keyspaceName, tableName);

Review Comment:
   I am not completely sure what you mean by that. The fact that you call this method, `importCompressionDictionary`, means that the table has dictionary compression enabled already, otherwise it would fail in `NodeProbe` in `getDictionaryManagerProxy`.
   
   What we can do though is to check that _kind_ of the dictionary which is meant to be imported matches the kind of the dictionary manager. It might happen that we would try to import `Kind.MY_DICT_ALGORITHM` while manager would be configured with `Kind.ZSTD` so it would not make any sense to import that.
   
   I think you basically meant this, correct?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490044105


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   We can do that but how do we differentiate between genuinely empty list and empty list as the result of some error? (as I assume we would return empty list when we catch an exception).
   
   If there is some bug in `CompressionDictionary.createFromRowLightweight`, or in the processing logic as such, and we throw, then we will happily return empty list and we will act as if nothing happened. 
   
   I would at least log in catch before returning empty list (perhaps in a non-spamming manner), same when returning null from other methods so we have at least some chance to see what went wrong. 
   
   Anyway, I think that having it like this here now is just fine :) I feel like logging it would be just too much. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2499079476


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   I think it does distinguish:
   
           List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspaceName, tableName);
           TabularDataSupport tableData = new TabularDataSupport(CompressionDictionaryDetailsTabularData.TABULAR_TYPE);
   
           if (dictionaries == null)
           {
               return tableData;
           }
   
           for (LightweightCompressionDictionary dictionary : dictionaries)
           {
   
   if it is null we return empty `tableData` otherwise we iterate.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490064610


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary
+    {
+        private final DictId dictId;
+        private final int checksum;
+        private final int size;
+
+        public LightweightCompressionDictionary(DictId dictId, int checksum, int size)
+        {
+            this.dictId = dictId;
+            this.checksum = checksum;
+            this.size = size;
+        }
+
+        @Override
+        public DictId dictId()
+        {
+            return dictId;
+        }
+
+        @Override
+        public byte[] rawDictionary()
+        {
+            return null;

Review Comment:
   null is valid value to return when we just list dictionaries as no actual data will be fetched from the database, we can return empty byte array for the sake of not returning null here and the in the composite / converting logic on listing I would return null if the value returned here is empty array.
   
   Throwing an exception is not a good idea because it would complicate the composite logic more unnecessarily. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2481393575


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -188,9 +220,10 @@ enum Kind
         // Order matters: the enum ordinal is serialized
         ZSTD
         {
-            public CompressionDictionary createDictionary(DictId dictId, byte[] dict)
+            @Override
+            public CompressionDictionary createDictionary(DictId dictId, byte[] dict, int checksum)
             {
-                return new ZstdCompressionDictionary(dictId, dict);
+                return new ZstdCompressionDictionary(dictId, dict, checksum);

Review Comment:
   This seems way more comfortable to do. If a reviewer looks into where checksum is worked with, we have it every time, no reason to not use that so we do not need to compute it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2515901627


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,304 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularType;
+
+import org.apache.cassandra.db.compression.CompressionDictionary.LightweightCompressionDictionary;
+import org.apache.cassandra.io.util.FileUtils;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    /**
+     * Position inside index names of tabular type of tabular data returned upon
+     * listing dictionaries where raw dictionary is expected to be located.
+     * We do not need to process this entry as listing does not contain any raw dictionary,
+     * only exporting does.
+     */
+    public static final int TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX = 3;
+
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * This method is meant to be call when listing dictionaries, we do not need actual dictionary byte arrays.
+     *
+     * @param dictionary lightweight dictionary to create composite data from
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromLightweightCompressionDictionary(LightweightCompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dictionary.keyspaceName,
+                                            dictionary.tableName,
+                                            dictionary.dictId.id,
+                                            null, // on purpose not returning actual dictionary
+                                            dictionary.dictId.kind.name(),
+                                            dictionary.checksum,
+                                            dictionary.size,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon exporting a dictionary.
+     *
+     * @param keyspace   keyspace of a dictionary
+     * @param table      table of a dictionary
+     * @param dictionary dictionary itself
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromCompressionDictionary(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.rawDictionary().length,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon deserialisation of data to composite data, e.g. upon importing a dictionary from JSON
+     * string to CompositeData before they are sent over JMX to import method.
+     *
+     * @param dataObject data class object to get composite data from
+     * @return composite data representing data class
+     */
+    public static CompositeData fromCompressionDictionaryDataObject(CompressionDictionaryDataObject dataObject)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dataObject.keyspace,
+                                            dataObject.table,
+                                            dataObject.dictId,
+                                            dataObject.dict,
+                                            dataObject.kind,
+                                            dataObject.dictChecksum,
+                                            dataObject.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create data object from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryDataObject#validate()
+     */
+    public static CompressionDictionaryDataObject fromCompositeData(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryDataObject dataObject = new CompressionDictionaryDataObject();
+        dataObject.keyspace = keyspace;
+        dataObject.table = table;
+        dataObject.dictId = dictId;
+        dataObject.dict = dictionaryBytes;
+        dataObject.kind = kind;
+        dataObject.dictChecksum = checksum;
+        dataObject.dictLength = size;
+
+        dataObject.validate();
+
+        return dataObject;
+    }
+
+    public static class CompressionDictionaryDataObject implements Cloneable
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * An object of this class is considered to be valid if:
+         *
+         * <ul>
+         *     <li>keyspace and table are not null</li>
+         *     <li>dict id is lower than 0</li>
+         *     <li>dict is not null nor empty</li>
+         *     <li>dict lenght is less than or equal to 1MiB</li>
+         *     <li>kind is not null and there is such {@link org.apache.cassandra.db.compression.CompressionDictionary.Kind}</li>
+         *     <li>dictLenght is bigger than 0</li>
+         *     <li>dictLenght has to be equal to dict's length</li>
+         *     <li>dictChecksum has to be equal to checksum computed as part of this method</li>
+         * </ul>
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");

Review Comment:
   The condition is `dictId <=0`, but the error message says `lower than 0`. The message is incorrect. Maybe change to "Dictionary id must be positive. The provided is {}"
   
   The ending `"'.'"` seems wrong too.



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,304 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularType;
+
+import org.apache.cassandra.db.compression.CompressionDictionary.LightweightCompressionDictionary;
+import org.apache.cassandra.io.util.FileUtils;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    /**
+     * Position inside index names of tabular type of tabular data returned upon
+     * listing dictionaries where raw dictionary is expected to be located.
+     * We do not need to process this entry as listing does not contain any raw dictionary,
+     * only exporting does.
+     */
+    public static final int TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX = 3;
+
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * This method is meant to be call when listing dictionaries, we do not need actual dictionary byte arrays.
+     *
+     * @param dictionary lightweight dictionary to create composite data from
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromLightweightCompressionDictionary(LightweightCompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dictionary.keyspaceName,
+                                            dictionary.tableName,
+                                            dictionary.dictId.id,
+                                            null, // on purpose not returning actual dictionary
+                                            dictionary.dictId.kind.name(),
+                                            dictionary.checksum,
+                                            dictionary.size,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon exporting a dictionary.
+     *
+     * @param keyspace   keyspace of a dictionary
+     * @param table      table of a dictionary
+     * @param dictionary dictionary itself
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromCompressionDictionary(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.rawDictionary().length,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon deserialisation of data to composite data, e.g. upon importing a dictionary from JSON
+     * string to CompositeData before they are sent over JMX to import method.
+     *
+     * @param dataObject data class object to get composite data from
+     * @return composite data representing data class
+     */
+    public static CompositeData fromCompressionDictionaryDataObject(CompressionDictionaryDataObject dataObject)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dataObject.keyspace,
+                                            dataObject.table,
+                                            dataObject.dictId,
+                                            dataObject.dict,
+                                            dataObject.kind,
+                                            dataObject.dictChecksum,
+                                            dataObject.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create data object from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryDataObject#validate()
+     */
+    public static CompressionDictionaryDataObject fromCompositeData(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryDataObject dataObject = new CompressionDictionaryDataObject();
+        dataObject.keyspace = keyspace;
+        dataObject.table = table;
+        dataObject.dictId = dictId;
+        dataObject.dict = dictionaryBytes;
+        dataObject.kind = kind;
+        dataObject.dictChecksum = checksum;
+        dataObject.dictLength = size;
+
+        dataObject.validate();
+
+        return dataObject;
+    }
+
+    public static class CompressionDictionaryDataObject implements Cloneable
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * An object of this class is considered to be valid if:
+         *
+         * <ul>
+         *     <li>keyspace and table are not null</li>
+         *     <li>dict id is lower than 0</li>
+         *     <li>dict is not null nor empty</li>
+         *     <li>dict lenght is less than or equal to 1MiB</li>

Review Comment:
   typo:
   
   ```suggestion
            *     <li>dict length is less than or equal to 1MiB</li>
   ```



##########
src/java/org/apache/cassandra/tools/NodeProbe.java:
##########
@@ -2698,18 +2699,86 @@ public void setMixedMajorVersionRepairEnabled(boolean enabled)
      */
     public void trainCompressionDictionary(String keyspace, String table, boolean force) throws IOException
     {
-        CompressionDictionaryManagerMBean proxy = getDictionaryManagerProxy(keyspace, table);
+        doWithCompressionDictionaryManagerMBean(proxy -> { proxy.train(force); return null; }, keyspace, table);

Review Comment:
   I think it is only my IDE being confused. It prompts that proxy could be null, which I do not believe it is true. 



##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionaryCommandGroup.java:
##########
@@ -0,0 +1,295 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.tools.nodetool;
+
+import java.io.PrintStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
+
+import com.google.common.util.concurrent.Uninterruptibles;
+
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryDataObject;
+import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
+import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+import picocli.CommandLine.Parameters;
+
+import static java.lang.System.lineSeparator;
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+import static java.util.stream.Collectors.joining;
+
+@Command(name = "compressiondictionary",
+         description = "Manage compression dictionaries",
+         subcommands = { CompressionDictionaryCommandGroup.TrainDictionary.class,
+                         CompressionDictionaryCommandGroup.ListDictionaries.class,
+                         CompressionDictionaryCommandGroup.ExportDictionary.class,
+                         CompressionDictionaryCommandGroup.ImportDictionary.class })
+public class CompressionDictionaryCommandGroup
+{
+    @Command(name = "train",
+             description = "Manually trigger compression dictionary training for a table. If no SSTables are available, the memtable will be flushed first.")
+    public static class TrainDictionary extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Option(names = { "-f", "--force" }, description = "Force the dictionary training even if there are not enough samples")
+        private boolean force = false;
+
+        @Override
+        public void execute(NodeProbe probe)
+        {
+            PrintStream out = probe.output().out;
+            PrintStream err = probe.output().err;
+
+            try
+            {
+                out.printf("Starting compression dictionary training for %s.%s...%n", keyspace, table);
+                out.printf("Training from existing SSTables (flushing first if needed)%n");
+
+                probe.trainCompressionDictionary(keyspace, table, force);
+
+                // Wait for training completion (10 minutes timeout for SSTable-based training)
+                out.println("Sampling from existing SSTables and training.");
+                long maxWaitMillis = TimeUnit.MINUTES.toMillis(10);
+                long startTime = Clock.Global.currentTimeMillis();
+
+                while (Clock.Global.currentTimeMillis() - startTime < maxWaitMillis)
+                {
+                    TrainingState trainingState = probe.getCompressionDictionaryTrainingState(keyspace, table);
+                    TrainingStatus status = trainingState.getStatus();
+                    displayProgress(trainingState, startTime, out, status);
+                    if (TrainingStatus.COMPLETED == status)
+                    {
+                        out.printf("%nTraining completed successfully for %s.%s%n", keyspace, table);
+                        return;
+                    }
+                    else if (TrainingStatus.FAILED == status)
+                    {
+                        err.printf("%nTraining failed for %s.%s%n", keyspace, table);
+                        try
+                        {
+                            String failureMessage = trainingState.getFailureMessage();
+                            if (failureMessage != null && !failureMessage.isEmpty())
+                            {
+                                err.printf("Reason: %s%n", failureMessage);
+                            }
+                        }
+                        catch (Exception e)
+                        {
+                            // If we can't get the failure message, just continue without it
+                        }
+                        System.exit(1);
+                    }
+
+                    Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+                }
+
+                err.printf("%nTraining did not complete within expected timeframe (10 minutes).%n");
+                System.exit(1);
+            }
+            catch (Exception e)
+            {
+                err.printf("Failed to trigger training: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+
+        private static void displayProgress(TrainingState trainingState, long startTime, PrintStream out, TrainingStatus status)
+        {
+            // Display meaningful statistics
+            long sampleCount = trainingState.getSampleCount();
+            long totalSampleSize = trainingState.getTotalSampleSize();
+            long elapsedSeconds = (Clock.Global.currentTimeMillis() - startTime) / 1000;
+            double sampleSizeMB = totalSampleSize / (1024.0 * 1024.0);
+
+            out.printf("\rStatus: %s | Samples: %d | Size: %.2f MiB | Elapsed: %ds",
+                       status, sampleCount, sampleSizeMB, elapsedSeconds);
+        }
+    }
+
+    @Command(name = "list",
+             description = "List available dictionaries of specific keyspace and table.")
+    public static class ListDictionaries extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Override
+        protected void execute(NodeProbe probe)
+        {
+            try
+            {
+                TableBuilder tableBuilder = new TableBuilder();
+                TabularData tabularData = probe.listCompressionDictionaries(keyspace, table);
+                List<String> indexNames = tabularData.getTabularType().getIndexNames();
+
+                List<String> columns = new ArrayList<>(indexNames);
+                // ignore raw dict
+                columns.remove(CompressionDictionaryDetailsTabularData.TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX);
+                tableBuilder.add(columns);
+
+                for (Object eachDict : tabularData.keySet())
+                {
+                    final List<?> dictRow = (List<?>) eachDict;
+
+                    List<String> rowValues = new ArrayList<>();
+
+                    for (int i = 0; i < dictRow.size(); i++)
+                    {
+                        if (i == 3) // ignore raw dict
+                            continue;
+
+                        rowValues.add(dictRow.get(i).toString());
+                    }
+                    tableBuilder.add(rowValues);
+                }
+
+                tableBuilder.printTo(probe.output().out);
+            }
+            catch (Exception e)
+            {
+                probe.output().err.printf("Failed to list dictionaries: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+    }
+
+    @Command(name = "export",
+             description = "Export dictionary from Cassandra to local file.")
+    public static class ExportDictionary extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Parameters(index = "2", description = "File name to save dictionary to", arity = "1")
+        private String dictionaryPath;
+
+        @Option(paramLabel = "dictId", names = { "-i", "--id" },
+        description = "The dictionary id. When not specified, the current dictionary is returned.")
+        private long dictId = -1;
+
+        @Override
+        protected void execute(NodeProbe probe)
+        {
+            if (dictId <= 0 && dictId != -1)
+            {
+                probe.output().err.printf("Dictionary id has to be strictly positive number.%n");
+                System.exit(1);
+            }
+
+            try
+            {
+                CompositeData compressionDictionary;
+
+                if (dictId == -1)
+                {
+                    compressionDictionary = probe.getCompressionDictionary(keyspace, table);
+                }
+                else
+                {
+                    compressionDictionary = probe.getCompressionDictionary(keyspace, table, dictId);
+                }
+
+                if (compressionDictionary == null)
+                {
+                    probe.output().err.printf("Dictionary does not exist.%n");
+                    System.exit(1);
+                }
+
+                CompressionDictionaryDataObject dataObject = CompressionDictionaryDetailsTabularData.fromCompositeData(compressionDictionary);
+                String dictionary = JsonUtils.writeAsPrettyJsonString(dataObject);
+                FileUtils.write(new File(dictionaryPath), List.of(dictionary), CREATE, TRUNCATE_EXISTING, WRITE);
+            }
+            catch (Throwable e)
+            {
+                probe.output().err.printf("Failed to export dictionary: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+    }
+
+    @Command(name = "import",
+             description = "Import local dictionary to Cassandra.")
+    public static class ImportDictionary extends AbstractCommand

Review Comment:
   Since import runs on the local node, there could still be concurrent imports from other nodes, even though we synchronize locally.
   We should probably document this somewhere. When imports happen from multiple nodes, the highest-version dictionary wins.



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,304 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularType;
+
+import org.apache.cassandra.db.compression.CompressionDictionary.LightweightCompressionDictionary;
+import org.apache.cassandra.io.util.FileUtils;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    /**
+     * Position inside index names of tabular type of tabular data returned upon
+     * listing dictionaries where raw dictionary is expected to be located.
+     * We do not need to process this entry as listing does not contain any raw dictionary,
+     * only exporting does.
+     */
+    public static final int TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX = 3;
+
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * This method is meant to be call when listing dictionaries, we do not need actual dictionary byte arrays.
+     *
+     * @param dictionary lightweight dictionary to create composite data from
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromLightweightCompressionDictionary(LightweightCompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dictionary.keyspaceName,
+                                            dictionary.tableName,
+                                            dictionary.dictId.id,
+                                            null, // on purpose not returning actual dictionary
+                                            dictionary.dictId.kind.name(),
+                                            dictionary.checksum,
+                                            dictionary.size,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon exporting a dictionary.
+     *
+     * @param keyspace   keyspace of a dictionary
+     * @param table      table of a dictionary
+     * @param dictionary dictionary itself
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromCompressionDictionary(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.rawDictionary().length,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon deserialisation of data to composite data, e.g. upon importing a dictionary from JSON
+     * string to CompositeData before they are sent over JMX to import method.
+     *
+     * @param dataObject data class object to get composite data from
+     * @return composite data representing data class
+     */
+    public static CompositeData fromCompressionDictionaryDataObject(CompressionDictionaryDataObject dataObject)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dataObject.keyspace,
+                                            dataObject.table,
+                                            dataObject.dictId,
+                                            dataObject.dict,
+                                            dataObject.kind,
+                                            dataObject.dictChecksum,
+                                            dataObject.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create data object from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryDataObject#validate()
+     */
+    public static CompressionDictionaryDataObject fromCompositeData(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryDataObject dataObject = new CompressionDictionaryDataObject();
+        dataObject.keyspace = keyspace;
+        dataObject.table = table;
+        dataObject.dictId = dictId;
+        dataObject.dict = dictionaryBytes;
+        dataObject.kind = kind;
+        dataObject.dictChecksum = checksum;
+        dataObject.dictLength = size;
+
+        dataObject.validate();
+
+        return dataObject;
+    }
+
+    public static class CompressionDictionaryDataObject implements Cloneable
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * An object of this class is considered to be valid if:
+         *
+         * <ul>
+         *     <li>keyspace and table are not null</li>
+         *     <li>dict id is lower than 0</li>
+         *     <li>dict is not null nor empty</li>
+         *     <li>dict lenght is less than or equal to 1MiB</li>
+         *     <li>kind is not null and there is such {@link org.apache.cassandra.db.compression.CompressionDictionary.Kind}</li>
+         *     <li>dictLenght is bigger than 0</li>
+         *     <li>dictLenght has to be equal to dict's length</li>
+         *     <li>dictChecksum has to be equal to checksum computed as part of this method</li>
+         * </ul>
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");
+            if (dict.length > FileUtils.ONE_MIB)
+                throw new IllegalArgumentException("Imported dictionary can not be larger than " +
+                                                   FileUtils.ONE_MIB + " bytes, but it is " +
+                                                   dict.length + " bytes.");
+            if (kind == null)
+                throw new IllegalArgumentException("Provided kind is null.");
+
+            CompressionDictionary.Kind dictionaryKind;
+
+            try
+            {
+                dictionaryKind = CompressionDictionary.Kind.valueOf(kind);
+            }
+            catch (IllegalArgumentException ex)
+            {
+                throw new IllegalArgumentException("There is no such dictionary kind like '" + kind + "'. Available kinds: " + Arrays.asList(CompressionDictionary.Kind.values()));
+            }
+
+            if (dictLength <= 0)
+                throw new IllegalArgumentException("Size has to be strictly positive number, it is '" + dictLength + "'.");
+            if (dict.length != dictLength)
+                throw new IllegalArgumentException("The length of the provided dictionary array (" + dict.length + ") is not equal to provided length value (" + dictLength + ").");
+
+            int checksumOfDictionaryToImport = CompressionDictionary.calculateChecksum((byte) dictionaryKind.ordinal(), dictId, dict);
+            if (checksumOfDictionaryToImport != dictChecksum)
+            {
+                throw new IllegalArgumentException(format("Computed checksum of dictionary to import (%s) is different from checksum specified on input (%s).",
+                                                          checksumOfDictionaryToImport,
+                                                          dictChecksum));
+            }
+        }
+
+        @Override
+        public CompressionDictionaryDataObject clone()
+        {
+            try
+            {
+                CompressionDictionaryDataObject clone = (CompressionDictionaryDataObject) super.clone();
+                if (this.dict != null)
+                {
+                    clone.dict = this.dict.clone();
+                }
+                return clone;
+            }
+            catch (CloneNotSupportedException e)
+            {
+                throw new AssertionError("Clone not supported", e);
+            }
+        }
+    }

Review Comment:
   Why is this method and `Cloneable` necessary? Looks like it is only used in test code `org.apache.cassandra.db.compression.CompressionDictionaryDataObjectTest#assertInvalid`



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,304 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularType;
+
+import org.apache.cassandra.db.compression.CompressionDictionary.LightweightCompressionDictionary;
+import org.apache.cassandra.io.util.FileUtils;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    /**
+     * Position inside index names of tabular type of tabular data returned upon
+     * listing dictionaries where raw dictionary is expected to be located.
+     * We do not need to process this entry as listing does not contain any raw dictionary,
+     * only exporting does.
+     */
+    public static final int TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX = 3;
+
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * This method is meant to be call when listing dictionaries, we do not need actual dictionary byte arrays.
+     *
+     * @param dictionary lightweight dictionary to create composite data from
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromLightweightCompressionDictionary(LightweightCompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dictionary.keyspaceName,
+                                            dictionary.tableName,
+                                            dictionary.dictId.id,
+                                            null, // on purpose not returning actual dictionary
+                                            dictionary.dictId.kind.name(),
+                                            dictionary.checksum,
+                                            dictionary.size,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon exporting a dictionary.
+     *
+     * @param keyspace   keyspace of a dictionary
+     * @param table      table of a dictionary
+     * @param dictionary dictionary itself
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromCompressionDictionary(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.rawDictionary().length,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon deserialisation of data to composite data, e.g. upon importing a dictionary from JSON
+     * string to CompositeData before they are sent over JMX to import method.
+     *
+     * @param dataObject data class object to get composite data from
+     * @return composite data representing data class
+     */
+    public static CompositeData fromCompressionDictionaryDataObject(CompressionDictionaryDataObject dataObject)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dataObject.keyspace,
+                                            dataObject.table,
+                                            dataObject.dictId,
+                                            dataObject.dict,
+                                            dataObject.kind,
+                                            dataObject.dictChecksum,
+                                            dataObject.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create data object from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryDataObject#validate()
+     */
+    public static CompressionDictionaryDataObject fromCompositeData(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryDataObject dataObject = new CompressionDictionaryDataObject();
+        dataObject.keyspace = keyspace;
+        dataObject.table = table;
+        dataObject.dictId = dictId;
+        dataObject.dict = dictionaryBytes;
+        dataObject.kind = kind;
+        dataObject.dictChecksum = checksum;
+        dataObject.dictLength = size;
+
+        dataObject.validate();
+
+        return dataObject;
+    }
+
+    public static class CompressionDictionaryDataObject implements Cloneable
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;

Review Comment:
   nit: add `final` modifier to those public fields. The change would require to add a constructor though



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,304 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularType;
+
+import org.apache.cassandra.db.compression.CompressionDictionary.LightweightCompressionDictionary;
+import org.apache.cassandra.io.util.FileUtils;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    /**
+     * Position inside index names of tabular type of tabular data returned upon
+     * listing dictionaries where raw dictionary is expected to be located.
+     * We do not need to process this entry as listing does not contain any raw dictionary,
+     * only exporting does.
+     */
+    public static final int TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX = 3;
+
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * This method is meant to be call when listing dictionaries, we do not need actual dictionary byte arrays.
+     *
+     * @param dictionary lightweight dictionary to create composite data from
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromLightweightCompressionDictionary(LightweightCompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dictionary.keyspaceName,
+                                            dictionary.tableName,
+                                            dictionary.dictId.id,
+                                            null, // on purpose not returning actual dictionary
+                                            dictionary.dictId.kind.name(),
+                                            dictionary.checksum,
+                                            dictionary.size,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon exporting a dictionary.
+     *
+     * @param keyspace   keyspace of a dictionary
+     * @param table      table of a dictionary
+     * @param dictionary dictionary itself
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromCompressionDictionary(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.rawDictionary().length,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon deserialisation of data to composite data, e.g. upon importing a dictionary from JSON
+     * string to CompositeData before they are sent over JMX to import method.
+     *
+     * @param dataObject data class object to get composite data from
+     * @return composite data representing data class
+     */
+    public static CompositeData fromCompressionDictionaryDataObject(CompressionDictionaryDataObject dataObject)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dataObject.keyspace,
+                                            dataObject.table,
+                                            dataObject.dictId,
+                                            dataObject.dict,
+                                            dataObject.kind,
+                                            dataObject.dictChecksum,
+                                            dataObject.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create data object from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryDataObject#validate()
+     */
+    public static CompressionDictionaryDataObject fromCompositeData(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryDataObject dataObject = new CompressionDictionaryDataObject();
+        dataObject.keyspace = keyspace;
+        dataObject.table = table;
+        dataObject.dictId = dictId;
+        dataObject.dict = dictionaryBytes;
+        dataObject.kind = kind;
+        dataObject.dictChecksum = checksum;
+        dataObject.dictLength = size;
+
+        dataObject.validate();
+
+        return dataObject;
+    }
+
+    public static class CompressionDictionaryDataObject implements Cloneable
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * An object of this class is considered to be valid if:
+         *
+         * <ul>
+         *     <li>keyspace and table are not null</li>
+         *     <li>dict id is lower than 0</li>
+         *     <li>dict is not null nor empty</li>
+         *     <li>dict lenght is less than or equal to 1MiB</li>
+         *     <li>kind is not null and there is such {@link org.apache.cassandra.db.compression.CompressionDictionary.Kind}</li>
+         *     <li>dictLenght is bigger than 0</li>
+         *     <li>dictLenght has to be equal to dict's length</li>

Review Comment:
   more length typos



##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionaryCommandGroup.java:
##########
@@ -0,0 +1,295 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.tools.nodetool;
+
+import java.io.PrintStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
+
+import com.google.common.util.concurrent.Uninterruptibles;
+
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryDataObject;
+import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
+import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+import picocli.CommandLine.Parameters;
+
+import static java.lang.System.lineSeparator;
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+import static java.util.stream.Collectors.joining;
+
+@Command(name = "compressiondictionary",
+         description = "Manage compression dictionaries",
+         subcommands = { CompressionDictionaryCommandGroup.TrainDictionary.class,
+                         CompressionDictionaryCommandGroup.ListDictionaries.class,
+                         CompressionDictionaryCommandGroup.ExportDictionary.class,
+                         CompressionDictionaryCommandGroup.ImportDictionary.class })
+public class CompressionDictionaryCommandGroup
+{
+    @Command(name = "train",
+             description = "Manually trigger compression dictionary training for a table. If no SSTables are available, the memtable will be flushed first.")
+    public static class TrainDictionary extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Option(names = { "-f", "--force" }, description = "Force the dictionary training even if there are not enough samples")
+        private boolean force = false;
+
+        @Override
+        public void execute(NodeProbe probe)
+        {
+            PrintStream out = probe.output().out;
+            PrintStream err = probe.output().err;
+
+            try
+            {
+                out.printf("Starting compression dictionary training for %s.%s...%n", keyspace, table);
+                out.printf("Training from existing SSTables (flushing first if needed)%n");
+
+                probe.trainCompressionDictionary(keyspace, table, force);
+
+                // Wait for training completion (10 minutes timeout for SSTable-based training)
+                out.println("Sampling from existing SSTables and training.");
+                long maxWaitMillis = TimeUnit.MINUTES.toMillis(10);
+                long startTime = Clock.Global.currentTimeMillis();
+
+                while (Clock.Global.currentTimeMillis() - startTime < maxWaitMillis)
+                {
+                    TrainingState trainingState = probe.getCompressionDictionaryTrainingState(keyspace, table);
+                    TrainingStatus status = trainingState.getStatus();
+                    displayProgress(trainingState, startTime, out, status);
+                    if (TrainingStatus.COMPLETED == status)
+                    {
+                        out.printf("%nTraining completed successfully for %s.%s%n", keyspace, table);
+                        return;
+                    }
+                    else if (TrainingStatus.FAILED == status)
+                    {
+                        err.printf("%nTraining failed for %s.%s%n", keyspace, table);
+                        try
+                        {
+                            String failureMessage = trainingState.getFailureMessage();
+                            if (failureMessage != null && !failureMessage.isEmpty())
+                            {
+                                err.printf("Reason: %s%n", failureMessage);
+                            }
+                        }
+                        catch (Exception e)
+                        {
+                            // If we can't get the failure message, just continue without it
+                        }
+                        System.exit(1);
+                    }
+
+                    Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+                }
+
+                err.printf("%nTraining did not complete within expected timeframe (10 minutes).%n");
+                System.exit(1);
+            }
+            catch (Exception e)
+            {
+                err.printf("Failed to trigger training: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+
+        private static void displayProgress(TrainingState trainingState, long startTime, PrintStream out, TrainingStatus status)
+        {
+            // Display meaningful statistics
+            long sampleCount = trainingState.getSampleCount();
+            long totalSampleSize = trainingState.getTotalSampleSize();
+            long elapsedSeconds = (Clock.Global.currentTimeMillis() - startTime) / 1000;
+            double sampleSizeMB = totalSampleSize / (1024.0 * 1024.0);
+
+            out.printf("\rStatus: %s | Samples: %d | Size: %.2f MiB | Elapsed: %ds",
+                       status, sampleCount, sampleSizeMB, elapsedSeconds);
+        }
+    }
+
+    @Command(name = "list",
+             description = "List available dictionaries of specific keyspace and table.")
+    public static class ListDictionaries extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Override
+        protected void execute(NodeProbe probe)
+        {
+            try
+            {
+                TableBuilder tableBuilder = new TableBuilder();
+                TabularData tabularData = probe.listCompressionDictionaries(keyspace, table);
+                List<String> indexNames = tabularData.getTabularType().getIndexNames();
+
+                List<String> columns = new ArrayList<>(indexNames);
+                // ignore raw dict
+                columns.remove(CompressionDictionaryDetailsTabularData.TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX);
+                tableBuilder.add(columns);
+
+                for (Object eachDict : tabularData.keySet())
+                {
+                    final List<?> dictRow = (List<?>) eachDict;
+
+                    List<String> rowValues = new ArrayList<>();
+
+                    for (int i = 0; i < dictRow.size(); i++)
+                    {
+                        if (i == 3) // ignore raw dict

Review Comment:
   Update the condition here as well.
   ```suggestion
                           if (i == CompressionDictionaryDetailsTabularData.TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX) // ignore raw dict
   ```



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -244,6 +251,68 @@ public CompositeData getTrainingState()
         return dictionaryTrainer.getTrainingState().toCompositeData();
     }
 
+    @Override
+    public TabularData listCompressionDictionaries()
+    {
+        List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspaceName, tableName);
+        TabularDataSupport tableData = new TabularDataSupport(CompressionDictionaryDetailsTabularData.TABULAR_TYPE);
+
+        if (dictionaries == null)
+        {
+            return tableData;
+        }
+
+        for (LightweightCompressionDictionary dictionary : dictionaries)
+        {
+            tableData.put(CompressionDictionaryDetailsTabularData.fromLightweightCompressionDictionary(dictionary));
+        }
+
+        return tableData;
+    }
+
+    @Override
+    public CompositeData getCompressionDictionary()
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveLatestCompressionDictionary(keyspaceName, tableName);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.fromCompressionDictionary(keyspaceName, tableName, compressionDictionary);
+    }
+
+    @Override
+    public CompositeData getCompressionDictionary(long dictId)
+    {
+        CompressionDictionary compressionDictionary = SystemDistributedKeyspace.retrieveCompressionDictionary(keyspaceName, tableName, dictId);
+        if (compressionDictionary == null)
+            return null;
+
+        return CompressionDictionaryDetailsTabularData.fromCompressionDictionary(keyspaceName, tableName, compressionDictionary);
+    }
+
+    @Override
+    public synchronized void importCompressionDictionary(CompositeData compositeData)
+    {
+        CompressionDictionaryDataObject dataObject = CompressionDictionaryDetailsTabularData.fromCompositeData(compositeData);
+
+        if (!keyspaceName.equals(dataObject.keyspace) || !tableName.equals(dataObject.table))
+            throw new IllegalArgumentException(format("Keyspace and table of a dictionary to import (%s.%s) does not correspond to the keyspace and table this manager is responsible for (%s.%s)",
+                                                      dataObject.keyspace, dataObject.table,
+                                                      keyspaceName, tableName));
+
+        CompressionDictionary.Kind kind = CompressionDictionary.Kind.valueOf(dataObject.kind);
+        CompressionDictionary.DictId dictId = new CompressionDictionary.DictId(kind, dataObject.dictId);
+
+        LightweightCompressionDictionary latestCompressionDictionary = SystemDistributedKeyspace.retrieveLightweightLatestCompressionDictionary(keyspaceName, tableName);

Review Comment:
   Should we check table has the correct dictionary compression enabled too?



##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionaryCommandGroup.java:
##########
@@ -0,0 +1,295 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.tools.nodetool;
+
+import java.io.PrintStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
+
+import com.google.common.util.concurrent.Uninterruptibles;
+
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryDataObject;
+import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
+import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+import picocli.CommandLine.Parameters;
+
+import static java.lang.System.lineSeparator;
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+import static java.util.stream.Collectors.joining;
+
+@Command(name = "compressiondictionary",
+         description = "Manage compression dictionaries",
+         subcommands = { CompressionDictionaryCommandGroup.TrainDictionary.class,
+                         CompressionDictionaryCommandGroup.ListDictionaries.class,
+                         CompressionDictionaryCommandGroup.ExportDictionary.class,
+                         CompressionDictionaryCommandGroup.ImportDictionary.class })
+public class CompressionDictionaryCommandGroup
+{
+    @Command(name = "train",
+             description = "Manually trigger compression dictionary training for a table. If no SSTables are available, the memtable will be flushed first.")
+    public static class TrainDictionary extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Option(names = { "-f", "--force" }, description = "Force the dictionary training even if there are not enough samples")
+        private boolean force = false;
+
+        @Override
+        public void execute(NodeProbe probe)
+        {
+            PrintStream out = probe.output().out;
+            PrintStream err = probe.output().err;
+
+            try
+            {
+                out.printf("Starting compression dictionary training for %s.%s...%n", keyspace, table);
+                out.printf("Training from existing SSTables (flushing first if needed)%n");
+
+                probe.trainCompressionDictionary(keyspace, table, force);
+
+                // Wait for training completion (10 minutes timeout for SSTable-based training)
+                out.println("Sampling from existing SSTables and training.");
+                long maxWaitMillis = TimeUnit.MINUTES.toMillis(10);
+                long startTime = Clock.Global.currentTimeMillis();
+
+                while (Clock.Global.currentTimeMillis() - startTime < maxWaitMillis)
+                {
+                    TrainingState trainingState = probe.getCompressionDictionaryTrainingState(keyspace, table);
+                    TrainingStatus status = trainingState.getStatus();
+                    displayProgress(trainingState, startTime, out, status);
+                    if (TrainingStatus.COMPLETED == status)
+                    {
+                        out.printf("%nTraining completed successfully for %s.%s%n", keyspace, table);
+                        return;
+                    }
+                    else if (TrainingStatus.FAILED == status)
+                    {
+                        err.printf("%nTraining failed for %s.%s%n", keyspace, table);
+                        try
+                        {
+                            String failureMessage = trainingState.getFailureMessage();
+                            if (failureMessage != null && !failureMessage.isEmpty())
+                            {
+                                err.printf("Reason: %s%n", failureMessage);
+                            }
+                        }
+                        catch (Exception e)
+                        {
+                            // If we can't get the failure message, just continue without it
+                        }
+                        System.exit(1);
+                    }
+
+                    Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+                }
+
+                err.printf("%nTraining did not complete within expected timeframe (10 minutes).%n");
+                System.exit(1);
+            }
+            catch (Exception e)
+            {
+                err.printf("Failed to trigger training: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+
+        private static void displayProgress(TrainingState trainingState, long startTime, PrintStream out, TrainingStatus status)
+        {
+            // Display meaningful statistics
+            long sampleCount = trainingState.getSampleCount();
+            long totalSampleSize = trainingState.getTotalSampleSize();
+            long elapsedSeconds = (Clock.Global.currentTimeMillis() - startTime) / 1000;
+            double sampleSizeMB = totalSampleSize / (1024.0 * 1024.0);
+
+            out.printf("\rStatus: %s | Samples: %d | Size: %.2f MiB | Elapsed: %ds",
+                       status, sampleCount, sampleSizeMB, elapsedSeconds);
+        }
+    }
+
+    @Command(name = "list",
+             description = "List available dictionaries of specific keyspace and table.")
+    public static class ListDictionaries extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Override
+        protected void execute(NodeProbe probe)
+        {
+            try
+            {
+                TableBuilder tableBuilder = new TableBuilder();
+                TabularData tabularData = probe.listCompressionDictionaries(keyspace, table);
+                List<String> indexNames = tabularData.getTabularType().getIndexNames();
+
+                List<String> columns = new ArrayList<>(indexNames);
+                // ignore raw dict
+                columns.remove(CompressionDictionaryDetailsTabularData.TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX);
+                tableBuilder.add(columns);
+
+                for (Object eachDict : tabularData.keySet())
+                {
+                    final List<?> dictRow = (List<?>) eachDict;
+
+                    List<String> rowValues = new ArrayList<>();
+
+                    for (int i = 0; i < dictRow.size(); i++)
+                    {
+                        if (i == 3) // ignore raw dict
+                            continue;
+
+                        rowValues.add(dictRow.get(i).toString());
+                    }
+                    tableBuilder.add(rowValues);
+                }
+
+                tableBuilder.printTo(probe.output().out);
+            }
+            catch (Exception e)
+            {
+                probe.output().err.printf("Failed to list dictionaries: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+    }
+
+    @Command(name = "export",
+             description = "Export dictionary from Cassandra to local file.")
+    public static class ExportDictionary extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Parameters(index = "2", description = "File name to save dictionary to", arity = "1")
+        private String dictionaryPath;
+
+        @Option(paramLabel = "dictId", names = { "-i", "--id" },
+        description = "The dictionary id. When not specified, the current dictionary is returned.")
+        private long dictId = -1;
+
+        @Override
+        protected void execute(NodeProbe probe)
+        {
+            if (dictId <= 0 && dictId != -1)
+            {
+                probe.output().err.printf("Dictionary id has to be strictly positive number.%n");
+                System.exit(1);
+            }
+
+            try
+            {
+                CompositeData compressionDictionary;
+
+                if (dictId == -1)
+                {
+                    compressionDictionary = probe.getCompressionDictionary(keyspace, table);
+                }
+                else
+                {
+                    compressionDictionary = probe.getCompressionDictionary(keyspace, table, dictId);
+                }
+
+                if (compressionDictionary == null)
+                {
+                    probe.output().err.printf("Dictionary does not exist.%n");

Review Comment:
   nit: adding a bit more information in the output
   ```suggestion
                       probe.output().err.printf("Dictionary%s does not exist for %s.%s.%n", 
                                                 dictId == -1 ? "" : " with id " + dictId, 
                                                 keyspace, table);
   ```



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -267,6 +336,11 @@ private void handleNewDictionary(CompressionDictionary dictionary)
         onNewDictionaryTrained(dictionary.dictId());
     }
 
+    public void importDictionary(CompressionDictionary dictionary)

Review Comment:
   ~this method could be `private`~
   
   Actually, should you just remove the method? I think the only call-site `CompressionDictionaryManager#importCompressionDictionary` can call `handleNewDictionary` instead. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2517783374


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,304 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularType;
+
+import org.apache.cassandra.db.compression.CompressionDictionary.LightweightCompressionDictionary;
+import org.apache.cassandra.io.util.FileUtils;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    /**
+     * Position inside index names of tabular type of tabular data returned upon
+     * listing dictionaries where raw dictionary is expected to be located.
+     * We do not need to process this entry as listing does not contain any raw dictionary,
+     * only exporting does.
+     */
+    public static final int TABULAR_DATA_TYPE_RAW_DICTIONARY_INDEX = 3;
+
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * This method is meant to be call when listing dictionaries, we do not need actual dictionary byte arrays.
+     *
+     * @param dictionary lightweight dictionary to create composite data from
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromLightweightCompressionDictionary(LightweightCompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dictionary.keyspaceName,
+                                            dictionary.tableName,
+                                            dictionary.dictId.id,
+                                            null, // on purpose not returning actual dictionary
+                                            dictionary.dictId.kind.name(),
+                                            dictionary.checksum,
+                                            dictionary.size,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon exporting a dictionary.
+     *
+     * @param keyspace   keyspace of a dictionary
+     * @param table      table of a dictionary
+     * @param dictionary dictionary itself
+     * @return composite data representing dictionary
+     */
+    public static CompositeData fromCompressionDictionary(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.rawDictionary().length,
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Used upon deserialisation of data to composite data, e.g. upon importing a dictionary from JSON
+     * string to CompositeData before they are sent over JMX to import method.
+     *
+     * @param dataObject data class object to get composite data from
+     * @return composite data representing data class
+     */
+    public static CompositeData fromCompressionDictionaryDataObject(CompressionDictionaryDataObject dataObject)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            dataObject.keyspace,
+                                            dataObject.table,
+                                            dataObject.dictId,
+                                            dataObject.dict,
+                                            dataObject.kind,
+                                            dataObject.dictChecksum,
+                                            dataObject.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create data object from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryDataObject#validate()
+     */
+    public static CompressionDictionaryDataObject fromCompositeData(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryDataObject dataObject = new CompressionDictionaryDataObject();
+        dataObject.keyspace = keyspace;
+        dataObject.table = table;
+        dataObject.dictId = dictId;
+        dataObject.dict = dictionaryBytes;
+        dataObject.kind = kind;
+        dataObject.dictChecksum = checksum;
+        dataObject.dictLength = size;
+
+        dataObject.validate();
+
+        return dataObject;
+    }
+
+    public static class CompressionDictionaryDataObject implements Cloneable
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;

Review Comment:
   This would complicate tests a bit etc. I have made it final but then I used builder to copy it and builder's methods to override any value.
   
   Builder is also validating that object and constructor of this class is made `private` so it can ever be constructed in the code by a builder only.
   
   JSON deserialisation from String to that object works too by adding appropriate annotations on that constructor. We need to deserialize from String (file) to an instance of that object when we go to import.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2489870485


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+        long dictId = (Long) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_ID_NAME);
+        byte[] dictionaryBytes = (byte[]) compositeData.get(CompressionDictionaryDetailsTabularData.DICT_NAME);
+        String kind = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KIND_NAME);
+        int checksum = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.CHECKSUM_NAME);
+        int size = (Integer) compositeData.get(CompressionDictionaryDetailsTabularData.SIZE_NAME);
+
+        CompressionDictionaryPojo pojo = new CompressionDictionaryPojo();
+        pojo.keyspace = keyspace;
+        pojo.table = table;
+        pojo.dictId = dictId;
+        pojo.dict = dictionaryBytes;
+        pojo.kind = kind;
+        pojo.dictChecksum = checksum;
+        pojo.dictLength = size;
+
+        pojo.validate();
+
+        return pojo;
+    }
+
+    public static class CompressionDictionaryPojo
+    {
+        public String keyspace;
+        public String table;
+        public long dictId;
+        public byte[] dict;
+        public String kind;
+        public int dictChecksum;
+        public int dictLength;
+
+        /**
+         * Dictionary is valid if, keyspace and table are specified, dictionary id is strictly positive integer,
+         * dictionary byte array is not nor not empty,
+         * kind corresponds to {@code Kind}, checksum and size are bigger than 0.
+         */
+        public void validate()
+        {
+            if (keyspace == null)
+                throw new IllegalArgumentException("keyspace not specified");
+            if (table == null)
+                throw new IllegalArgumentException("table not specified");
+            if (dictId <= 0)
+                throw new IllegalArgumentException("Provided dictionary id is lower than 0, it is '" + dictId + "'.'");
+            if (dict == null || dict.length == 0)
+                throw new IllegalArgumentException("Provided dictionary byte array is null or empty.");
+            if (kind == null)
+                throw new IllegalArgumentException("Provided kind is null.");
+
+            CompressionDictionary.Kind dictionarykind;
+
+            try
+            {
+                dictionarykind = CompressionDictionary.Kind.valueOf(kind);
+            }
+            catch (IllegalArgumentException ex)
+            {
+                throw new IllegalArgumentException("There is no such dictionary kind like '" + kind + "'. Available kinds: " + Arrays.asList(CompressionDictionary.Kind.values()));
+            }
+
+            if (dictChecksum <= 0)
+                throw new IllegalArgumentException("Checksum has to be strictly positive number, it is '" + dictChecksum + "'.");

Review Comment:
   aha interesting, good to know. I have never seen it negative so far. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2499079476


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   I think it does distinguishes:
   
           List<LightweightCompressionDictionary> dictionaries = SystemDistributedKeyspace.retrieveLightweightCompressionDictionaries(keyspaceName, tableName);
           TabularDataSupport tableData = new TabularDataSupport(CompressionDictionaryDetailsTabularData.TABULAR_TYPE);
   
           if (dictionaries == null)
           {
               return tableData;
           }
   
           for (LightweightCompressionDictionary dictionary : dictionaries)
           {
   
   if it is null we return empty `tableData` otherwise we iterate.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2481320229


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -188,9 +220,10 @@ enum Kind
         // Order matters: the enum ordinal is serialized
         ZSTD
         {
-            public CompressionDictionary createDictionary(DictId dictId, byte[] dict)
+            @Override
+            public CompressionDictionary createDictionary(DictId dictId, byte[] dict, int checksum)

Review Comment:
   This seems way more comfortable to do. If a reviewer looks into where checksum is worked with, we have it every time, no reason to not use that so we do not need to compute it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2494325366


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary

Review Comment:
   @yifan-c I have rewritten that to simple `LightweightCompressionDictionary`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2495995199


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   `org.apache.cassandra.db.compression.CompressionDictionaryManager#listCompressionDictionaries` is the only consumer of the method. Its outcome does not distinguish between whether the return value is null or empty list. Anyway, it is minor and I'd just defer to you. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2496063572


##########
src/java/org/apache/cassandra/tools/NodeProbe.java:
##########
@@ -2697,11 +2698,87 @@ public void setMixedMajorVersionRepairEnabled(boolean enabled)
      * @throws IllegalArgumentException if table doesn't support dictionary compression
      */
     public void trainCompressionDictionary(String keyspace, String table, boolean force) throws IOException
+    {
+        doWithCompressionDictionaryManagerMBean(proxy -> { proxy.train(force); return null; }, keyspace, table);
+    }
+
+    /**
+     * Returns latest dictionary for given keyspace and table.
+     *
+     * @param keyspace the keyspace name
+     * @param table the table name
+     * @return the latest dictionary for given keyspace and table
+     * @throws IOException if there's an error accessing the MBean
+     * @throws IllegalArgumentException if table doesn't support dictionary compression
+     */
+    public CompositeData getCompressionDictionary(String keyspace, String table) throws IOException
+    {
+        return doWithCompressionDictionaryManagerMBean(CompressionDictionaryManagerMBean::getCompressionDictionary, keyspace, table);
+    }
+
+    /**
+     * Returns the dictionary for given keyspace and table and dictionary id.
+     *
+     * @param keyspace the keyspace name
+     * @param table the table name
+     * @param dictId id of dictionary to get
+     * @return the dictionary for given keyspace and table and dictionary id.
+     * @throws IOException if there's an error accessing the MBean
+     * @throws IllegalArgumentException if table doesn't support dictionary compression
+     */
+    public CompositeData getCompressionDictionary(String keyspace, String table, long dictId) throws IOException
+    {
+        return doWithCompressionDictionaryManagerMBean(proxy -> proxy.getCompressionDictionary(dictId), keyspace, table);
+    }
+
+    /**
+     * Imports dictionary in composite data to database.
+     *
+     * @param compositeData data to import
+     * @throws IOException if there's an error accessing the MBean
+     * @throws IllegalArgumentException if table doesn't support dictionary compression
+     */
+    public void importCompressionDictionary(CompositeData compositeData) throws IOException
+    {
+        String keyspace = (String) compositeData.get(CompressionDictionaryDetailsTabularData.KEYSPACE_NAME);
+        String table = (String) compositeData.get(CompressionDictionaryDetailsTabularData.TABLE_NAME);
+
+        if (keyspace == null || table == null)
+        {
+            throw new IllegalStateException("Argument must have keyspace and table values.");
+        }
+
+        doWithCompressionDictionaryManagerMBean(proxy -> { proxy.importDictionary(compositeData); return null; }, keyspace, table);
+    }
+
+    private <T> T doWithCompressionDictionaryManagerMBean(Function<CompressionDictionaryManagerMBean, T> func,
+                                                          String keyspace, String table) throws IOException
+    {
+        try
+        {
+            return func.apply(getDictionaryManagerProxy(keyspace, table));
+        }
+        catch (Exception e)
+        {
+            if (e.getCause() instanceof InstanceNotFoundException)
+            {
+                String message = String.format("Table %s.%s does not exist or does not support dictionary compression",
+                                               keyspace, table);
+                throw new IllegalArgumentException(message);
+            }
+            else
+            {
+                throw new IOException(e.getMessage());
+            }
+        }
+    }
+
+    public TabularData listCompressionDictionaries(String keyspace, String table) throws IOException
     {
         CompressionDictionaryManagerMBean proxy = getDictionaryManagerProxy(keyspace, table);
         try
         {
-            proxy.train(force);
+            return proxy.listCompressionDictionaries();

Review Comment:
   Looks like `doWithCompressionDictionaryManagerMBean` is suitable for this method too.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2496046776


##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionaryCommandGroup.java:
##########
@@ -0,0 +1,307 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.tools.nodetool;
+
+import java.io.PrintStream;
+import java.util.List;
+import java.util.ArrayList;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
+
+import com.google.common.util.concurrent.Uninterruptibles;
+
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryDataObject;
+import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
+import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+import picocli.CommandLine.Parameters;
+
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
+@Command(name = "compressiondictionary",
+         description = "Manage compression dictionaries",
+         subcommands = { CompressionDictionaryCommandGroup.TrainDictionary.class,
+                         CompressionDictionaryCommandGroup.ListDictionaries.class,
+                         CompressionDictionaryCommandGroup.ExportDictionary.class,
+                         CompressionDictionaryCommandGroup.ImportDictionary.class })
+public class CompressionDictionaryCommandGroup
+{
+    @Command(name = "train",
+             description = "Manually trigger compression dictionary training for a table. If no SSTables are available, the memtable will be flushed first.")
+    public static class TrainDictionary extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Option(names = { "-f", "--force" }, description = "Force the dictionary training even if there are not enough samples")
+        private boolean force = false;
+
+        @Override
+        public void execute(NodeProbe probe)
+        {
+            PrintStream out = probe.output().out;
+            PrintStream err = probe.output().err;
+
+            try
+            {
+                out.printf("Starting compression dictionary training for %s.%s...%n", keyspace, table);
+                out.printf("Training from existing SSTables (flushing first if needed)%n");
+
+                probe.trainCompressionDictionary(keyspace, table, force);
+
+                // Wait for training completion (10 minutes timeout for SSTable-based training)
+                out.println("Sampling from existing SSTables and training.");
+                long maxWaitMillis = TimeUnit.MINUTES.toMillis(10);
+                long startTime = Clock.Global.currentTimeMillis();
+
+                while (Clock.Global.currentTimeMillis() - startTime < maxWaitMillis)
+                {
+                    TrainingState trainingState = probe.getCompressionDictionaryTrainingState(keyspace, table);
+                    TrainingStatus status = trainingState.getStatus();
+                    displayProgress(trainingState, startTime, out, status);
+                    if (TrainingStatus.COMPLETED == status)
+                    {
+                        out.printf("%nTraining completed successfully for %s.%s%n", keyspace, table);
+                        return;
+                    }
+                    else if (TrainingStatus.FAILED == status)
+                    {
+                        err.printf("%nTraining failed for %s.%s%n", keyspace, table);
+                        try
+                        {
+                            String failureMessage = trainingState.getFailureMessage();
+                            if (failureMessage != null && !failureMessage.isEmpty())
+                            {
+                                err.printf("Reason: %s%n", failureMessage);
+                            }
+                        }
+                        catch (Exception e)
+                        {
+                            // If we can't get the failure message, just continue without it
+                        }
+                        System.exit(1);
+                    }
+
+                    Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+                }
+
+                err.printf("%nTraining did not complete within expected timeframe (10 minutes).%n");
+                System.exit(1);
+            }
+            catch (Exception e)
+            {
+                err.printf("Failed to trigger training: %s%n", e.getMessage());
+                System.exit(1);
+            }
+        }
+
+        private static void displayProgress(TrainingState trainingState, long startTime, PrintStream out, TrainingStatus status)
+        {
+            // Display meaningful statistics
+            long sampleCount = trainingState.getSampleCount();
+            long totalSampleSize = trainingState.getTotalSampleSize();
+            long elapsedSeconds = (Clock.Global.currentTimeMillis() - startTime) / 1000;
+            double sampleSizeMB = totalSampleSize / (1024.0 * 1024.0);
+
+            out.printf("\rStatus: %s | Samples: %d | Size: %.2f MiB | Elapsed: %ds",
+                       status, sampleCount, sampleSizeMB, elapsedSeconds);
+        }
+    }
+
+    @Command(name = "list",
+             description = "List available dictionaries of specific keyspace and table.")
+    public static class ListDictionaries extends AbstractCommand
+    {
+        @Parameters(index = "0", description = "The keyspace name", arity = "1")
+        private String keyspace;
+
+        @Parameters(index = "1", description = "The table name", arity = "1")
+        private String table;
+
+        @Override
+        protected void execute(NodeProbe probe)
+        {
+            try
+            {
+                TableBuilder tableBuilder = new TableBuilder();
+                TabularData tabularData = probe.listCompressionDictionaries(keyspace, table);
+                List<String> indexNames = tabularData.getTabularType().getIndexNames();
+
+                List<String> columns = new ArrayList<>(indexNames);
+                columns.remove(3);  // ignore raw dict

Review Comment:
   nit: how about defining a constant for the index and ideally it is defined in `CompressionDictionaryDetailsTabularData`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2487676146


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData
+{
+    public static final String KEYSPACE_NAME = "Keyspace";
+    public static final String TABLE_NAME = "Table";
+    public static final String DICT_ID_NAME = "DictId";
+    public static final String DICT_NAME = "Dict";
+    public static final String KIND_NAME = "Kind";
+    public static final String CHECKSUM_NAME = "Checksum";
+    public static final String SIZE_NAME = "Size";
+
+
+    private static final String[] ITEM_NAMES = new String[]{ KEYSPACE_NAME,
+                                                             TABLE_NAME,
+                                                             DICT_ID_NAME,
+                                                             DICT_NAME,
+                                                             KIND_NAME,
+                                                             CHECKSUM_NAME,
+                                                             SIZE_NAME };
+
+    private static final String[] ITEM_DESCS = new String[]{ "keyspace",
+                                                             "table",
+                                                             "dictionary_id",
+                                                             "dictionary_bytes",
+                                                             "kind",
+                                                             "checksum",
+                                                             "size" };
+
+    private static final String TYPE_NAME = "DictionaryDetails";
+    private static final String ROW_DESC = "DictionaryDetails";
+    private static final OpenType<?>[] ITEM_TYPES;
+    private static final CompositeType COMPOSITE_TYPE;
+    public static final TabularType TABULAR_TYPE;
+
+    static
+    {
+        try
+        {
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, // keyspace
+                                         SimpleType.STRING, // table
+                                         SimpleType.LONG, // dict id
+                                         new ArrayType<String[]>(SimpleType.BYTE, true), // dict bytes
+                                         SimpleType.STRING, // kind
+                                         SimpleType.INTEGER, // checksum
+                                         SimpleType.INTEGER }; // size of dict bytes
+
+            COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
+            TABULAR_TYPE = new TabularType(TYPE_NAME, ROW_DESC, COMPOSITE_TYPE, ITEM_NAMES);
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static void from(String keyspace,
+                            String table,
+                            CompressionDictionary dictionary,
+                            TabularDataSupport result)
+    {
+        result.put(from(keyspace, table, dictionary));
+    }
+
+    public static CompositeData from(String keyspace, String table, CompressionDictionary dictionary)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            keyspace,
+                                            table,
+                                            dictionary.dictId().id,
+                                            dictionary.rawDictionary(),
+                                            dictionary.kind().name(),
+                                            dictionary.checksum(),
+                                            dictionary.size(),
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static CompositeData from(CompressionDictionaryPojo pojo)
+    {
+        try
+        {
+            return new CompositeDataSupport(COMPOSITE_TYPE,
+                                            ITEM_NAMES,
+                                            new Object[]
+                                            {
+                                            pojo.keyspace,
+                                            pojo.table,
+                                            pojo.dictId,
+                                            pojo.dict,
+                                            pojo.kind,
+                                            pojo.dictChecksum,
+                                            pojo.dictLength
+                                            });
+        }
+        catch (OpenDataException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Deserializes data to convenience object to work further with.
+     *
+     * @param compositeData data to create pojo from
+     * @return deserialized composite data to convenience object
+     * @throws IllegalArgumentException if values in deserialized object are invalid.
+     * @see CompressionDictionaryPojo#validate()
+     */
+    public static CompressionDictionaryPojo from(CompositeData compositeData)

Review Comment:
   Why both methods are named "from"? They reads confusing. 
   ```
       public static CompositeData from(CompressionDictionaryPojo pojo)
       public static CompressionDictionaryPojo from(CompositeData compositeData)
   ```



##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -18,25 +18,42 @@
 package org.apache.cassandra.tools.nodetool;
 
 import java.io.PrintStream;
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
 
 import com.google.common.util.concurrent.Uninterruptibles;
 
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryPojo;
 import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
 import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
 import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
 @Command(name = "compressiondictionary",
-         description = "Manage compression dictionaries",
-         subcommands = { CompressionDictionary.Train.class })
+description = "Manage compression dictionaries",
+subcommands = { CompressionDictionary.Train.class,
+                CompressionDictionary.List.class,
+                CompressionDictionary.Export.class,
+                CompressionDictionary.Import.class })

Review Comment:
   can we add test cases for the new commands? 



##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryDetailsTabularData.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.compression;
+
+import java.util.Arrays;
+import javax.management.openmbean.ArrayType;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
+
+import static java.lang.String.format;
+
+public class CompressionDictionaryDetailsTabularData

Review Comment:
   Could you add tests for this class? 
   1. conversion between CompressionDictionary and CompositeData
   2. validation 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490064610


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -281,4 +315,60 @@ public String toString()
                    '}';
         }
     }
+
+    /**
+     * The purpose of lightweight dictionary is to not carry the actual dictionary bytes for performance reasons.
+     * Handy for situations when retrieval from the database does not need to contain dictionary
+     * or the instatiation of a proper dictionary object is not desirable or unnecessary for other,
+     * mostly performance-related, reasons.
+     */
+    class LightweightCompressionDictionary implements CompressionDictionary
+    {
+        private final DictId dictId;
+        private final int checksum;
+        private final int size;
+
+        public LightweightCompressionDictionary(DictId dictId, int checksum, int size)
+        {
+            this.dictId = dictId;
+            this.checksum = checksum;
+            this.size = size;
+        }
+
+        @Override
+        public DictId dictId()
+        {
+            return dictId;
+        }
+
+        @Override
+        public byte[] rawDictionary()
+        {
+            return null;

Review Comment:
   null is valid value to return when we just list dictionaries as no actual data will be fetched from the database, we can return empty byte array for the sake of not returning null here and then in the composite / converting logic on listing I would return null if the value returned here is empty array.
   
   Throwing an exception is not a good idea because it would complicate the composite logic more unnecessarily. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490243155


##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -18,26 +18,44 @@
 package org.apache.cassandra.tools.nodetool;
 
 import java.io.PrintStream;
+import java.util.List;
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
 
 import com.google.common.util.concurrent.Uninterruptibles;
 
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryPojo;
 import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
 import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
 import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
 @Command(name = "compressiondictionary",
-         description = "Manage compression dictionaries",
-         subcommands = { CompressionDictionary.Train.class })
+description = "Manage compression dictionaries",
+subcommands = { CompressionDictionary.TrainDictionary.class,
+                CompressionDictionary.ListDictionaries.class,
+                CompressionDictionary.ExportDictionary.class,
+                CompressionDictionary.ImportDictionary.class })
 public class CompressionDictionary

Review Comment:
   @yifan-c I would rename this to `CompressionDictionaryCommand` or similar, maybe `CompressionDictionartyCommandGroup`.
   
   There is already` CompressionDictionary` on server side and it is confusing to differentiate between each other when coding. I struggle to find the current one when they have same names, even they are in different packages. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490238235


##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -18,26 +18,44 @@
 package org.apache.cassandra.tools.nodetool;
 
 import java.io.PrintStream;
+import java.util.List;
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
 
 import com.google.common.util.concurrent.Uninterruptibles;
 
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryPojo;
 import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
 import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
 import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
 @Command(name = "compressiondictionary",
-         description = "Manage compression dictionaries",
-         subcommands = { CompressionDictionary.Train.class })
+description = "Manage compression dictionaries",
+subcommands = { CompressionDictionary.TrainDictionary.class,
+                CompressionDictionary.ListDictionaries.class,

Review Comment:
   I did not like that only List would be renamed to ListDictionaries and it would be there like a sore thumb among others so I renamed the rest of them as well to be consistent in it at least. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490250696


##########
src/java/org/apache/cassandra/tools/nodetool/CompressionDictionary.java:
##########
@@ -18,26 +18,44 @@
 package org.apache.cassandra.tools.nodetool;
 
 import java.io.PrintStream;
+import java.util.List;
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import javax.management.openmbean.CompositeData;
+import javax.management.openmbean.TabularData;
 
 import com.google.common.util.concurrent.Uninterruptibles;
 
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData;
+import org.apache.cassandra.db.compression.CompressionDictionaryDetailsTabularData.CompressionDictionaryPojo;
 import org.apache.cassandra.db.compression.ICompressionDictionaryTrainer.TrainingStatus;
 import org.apache.cassandra.db.compression.TrainingState;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
 import org.apache.cassandra.utils.Clock;
+import org.apache.cassandra.utils.JsonUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 
+import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
+import static java.nio.file.StandardOpenOption.WRITE;
+
 @Command(name = "compressiondictionary",
-         description = "Manage compression dictionaries",
-         subcommands = { CompressionDictionary.Train.class })
+description = "Manage compression dictionaries",
+subcommands = { CompressionDictionary.TrainDictionary.class,
+                CompressionDictionary.ListDictionaries.class,

Review Comment:
   I will align it in the other commit. I think these comments would be gone if I do it now.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2489814840


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionary.java:
##########
@@ -49,6 +49,20 @@ public interface CompressionDictionary extends AutoCloseable
      */
     byte[] rawDictionary();
 
+    /**
+     * Get checkum of this dictionary.
+     *
+     * @return checksum of this dictionary
+     */
+    int checksum();
+
+    /**
+     * Get size of raw dictionary.
+     *
+     * @return size of raw dictionary, in bytes
+     */
+    int size();

Review Comment:
   because when this is used in lightweight version, rawDictionary() will return null. Then when converting it to CompositeData on dictionary listing, I can't do `rawDictionary().length` on it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2490044105


##########
src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java:
##########
@@ -481,6 +509,38 @@ public static CompressionDictionary retrieveCompressionDictionary(String keyspac
         }
     }
 
+    /**
+     * Retrieves all dictionaries for a given keyspace and table.
+     *
+     * @param keyspaceName the keyspace name to retrieve the dictionary for
+     * @param tableName the table name to retrieve the dictionary for
+     * @return the compression dictionaries identified by the specified keyspace and table,
+     *         or null if no dictionary exists or if an error occurs during retrieval
+     */
+    @Nullable

Review Comment:
   We can do that but how do we differentiate between genuinely empty list and empty list as the result of some error? (as I assume we would return empty list when we catch an exception).
   
   If there is some bug in `CompressionDictionary.createFromRowLightweight`, or in the processing logic as such, and we throw, then we will happily return empty list and we will act as if nothing happened. 
   
   I would at least log in catch before returning empty list (perhaps in a non-spamming manner), same when returning null from other methods so we have at least some chance to see what went wrong. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#issuecomment-3472963921

   tests not added yet, too lazy to add them on this stage when prod code / approach is not reviewed yet and might change significantly


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Re: [PR] CASSANDRA-20941 Add nodetool command to import / export/ list compression dictionary [cassandra]

Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4458:
URL: https://github.com/apache/cassandra/pull/4458#discussion_r2517785286


##########
src/java/org/apache/cassandra/db/compression/CompressionDictionaryManager.java:
##########
@@ -267,6 +336,11 @@ private void handleNewDictionary(CompressionDictionary dictionary)
         onNewDictionaryTrained(dictionary.dictId());
     }
 
+    public void importDictionary(CompressionDictionary dictionary)

Review Comment:
   yes I have noticed this too.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org