pkolaczk commented on code in PR #4038:
URL: https://github.com/apache/cassandra/pull/4038#discussion_r2031084418


##########
src/java/org/apache/cassandra/schema/TableMetadata.java:
##########
@@ -688,7 +693,40 @@ public ClusteringComparator 
partitionKeyAsClusteringComparator()
     public String indexTableName(IndexMetadata info)
     {
         // TODO simplify this when info.index_name is guaranteed to be set
-        return name + Directories.SECONDARY_INDEX_NAME_SEPARATOR + info.name;
+        return name + SECONDARY_INDEX_NAME_SEPARATOR + info.name;
+    }
+
+    /**
+     * Returns the table part of the index table name or the entire table name
+     * if not an index table.
+     * @return table name part
+     */
+    public String getTableName()
+    {
+        int idx = name.indexOf(SECONDARY_INDEX_NAME_SEPARATOR);
+        return idx >= 0 ? name.substring(0, idx) : name;
+    }
+
+    /**
+     * Generates directory name for the table by using table part of
+     * the (index) table name and table id.
+     * @return directory name
+     */
+    public String getTableDirectoryName()
+    {
+        return getTableName() + '-' + id.toHexString();
+    }
+
+    /**
+     * Gets the index name from the name of the index table including dot 
prefix.

Review Comment:
   This is unclear to me. 
   What is the "dot prefix"? Just the dot or also what's before the dot (that 
is, the table name).
   Maybe you wanted to say "returns the index name prefixed by a dot or null if 
not an index table" ?
   
   However, I'm not sure what's the point of dedicating a whole method for 
returning the index name with the leading dot. Prefixing with dot looks like 
something that should be done by the caller.



##########
src/java/org/apache/cassandra/schema/SchemaConstants.java:
##########
@@ -75,14 +77,54 @@ public final class SchemaConstants
      */
     public static final int NAME_LENGTH = 48;
 
+    /**
+     * Longest acceptable file name. Longer names lead to too long file name 
error.
+     */
+    public static final int FILENAME_LENGTH = 255;
+
+    /**
+     * Longest acceptable table name, so it can be used in a directory
+     * name constructed with a suffix of a table id and a separator.
+     */
+    public static final int TABLE_NAME_LENGTH = FILENAME_LENGTH - 32 - 1;
+
     // 59adb24e-f3cd-3e02-97f0-5b395827453f
     public static final UUID emptyVersion;
 
     public static final List<String> LEGACY_AUTH_TABLES = 
Arrays.asList("credentials", "users", "permissions");
 
+    /**
+     * Validates that a name is valid to be used in files. Assumes that the 
name length
+     * should fit the default, {@link #NAME_LENGTH}.
+     * See {@link #isValidName(String, int)} for more details.
+     *
+     * @param name the name to check
+     * @return whether the name is safe for use in file paths and file names
+     */
     public static boolean isValidName(String name)
     {
-        return name != null && !name.isEmpty() && name.length() <= NAME_LENGTH 
&& PATTERN_WORD_CHARS.matcher(name).matches();
+        return isValidName(name, NAME_LENGTH);
+    }
+
+    /**
+     * Names such as keyspace, table, index names are used in file paths and 
file names,
+     * so, they need to be safe for the use there, i.e., short enough and
+     * containing only alphanumeric characters and underscores.
+     * Allows to provide the length of names, since it varies for different 
database objects,
+     * especially, they were not historically controlled for {@link 
#NAME_LENGTH}, see,
+     * e.g., CASSANDRA-20389.
+     * There are cases when the length cannot be controlled by a single value. 
In such case
+     * the length validation is skipped if the given length is smaller than 1.
+     *
+     * @param name      the name to check
+     * @param maxLength max acceptable length for the given name. For the 
cases when it cannot
+     *                  be limited, 0 or negative number should be supplied.

Review Comment:
   For the cases where it *cannot* be limited - did you mean "for the cases 
where it *should* not be limited" ?
   Also, specifying zero doesn't look like natural value. Why not specify 
`Integer.MAX_VALUE` for unlimited and assert maxLength is > 0? 



-- 
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-unsubscr...@cassandra.apache.org

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


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

Reply via email to