swuferhong commented on code in PR #1706:
URL: https://github.com/apache/fluss/pull/1706#discussion_r2371276886
##########
fluss-common/src/main/java/org/apache/fluss/metadata/TablePath.java:
##########
@@ -127,20 +129,35 @@ public String toString() {
public static void validateDatabaseName(String databaseName) throws
InvalidDatabaseException {
String dbError = detectInvalidName(databaseName);
- if (dbError != null) {
+ String dbInternalNameError = validatePrefix(databaseName);
+ if (dbError != null || dbInternalNameError != null) {
throw new InvalidDatabaseException(
- "Database name " + databaseName + " is invalid: " +
dbError);
+ "Database name "
+ + databaseName
+ + " is invalid: "
+ + (dbError != null ? dbError :
dbInternalNameError));
}
}
public static void validateTableName(String tableName) throws
InvalidTableException {
String tableError = detectInvalidName(tableName);
- if (tableError != null) {
+ String tableInternalNameError = validatePrefix(tableName);
+ if (tableError != null || tableInternalNameError != null) {
throw new InvalidTableException(
- "Table name " + tableName + " is invalid: " + tableError);
+ "Table name "
+ + tableName
+ + " is invalid: "
+ + (tableError != null ? tableError :
tableInternalNameError));
}
}
+ public static String validatePrefix(String identifier) throws
InvalidTableException {
+ if (identifier != null && identifier.startsWith(INTERNAL_NAME_PREFIX))
{
+ return "'" + INTERNAL_NAME_PREFIX + "' is not allowed as prefix";
Review Comment:
The error message appears too simple. Could we include an explanation of why
'__' cannot be used, since '__' is reserved for internal tables/internal
partitions in Fluss server?
##########
fluss-common/src/test/java/org/apache/fluss/utils/PartitionUtilsTest.java:
##########
@@ -52,18 +52,23 @@ class PartitionUtilsTest {
@Test
void testValidatePartitionValues() {
- assertThatThrownBy(() -> validatePartitionValues(Arrays.asList("$1",
"2")))
+ assertThatThrownBy(() -> validatePartitionValues(Arrays.asList("$1",
"2"), true))
.isInstanceOf(InvalidPartitionException.class)
.hasMessageContaining(
"The partition value $1 is invalid: '$1' contains one "
+ "or more characters other than ASCII
alphanumerics, '_' and '-'");
- assertThatThrownBy(() -> validatePartitionValues(Arrays.asList("?1",
"2")))
+ assertThatThrownBy(() -> validatePartitionValues(Arrays.asList("?1",
"2"), false))
.isInstanceOf(InvalidPartitionException.class)
.hasMessageContaining(
"The partition value ?1 is invalid: '?1' contains one
or more "
+ "characters other than ASCII alphanumerics,
'_' and '-'");
+ assertThatThrownBy(() -> validatePartitionValues(Arrays.asList("__p1",
"2"), true))
+ .isInstanceOf(InvalidPartitionException.class)
+ .hasMessageContaining(
+ "The partition value __p1 is invalid: '__' is not
allowed as prefix");
+
Review Comment:
add ut when `isCreate` equals `false`.
##########
fluss-common/src/main/java/org/apache/fluss/metadata/TablePath.java:
##########
@@ -54,6 +54,8 @@ public class TablePath implements Serializable {
// RecordAccumulator.ready)
private Integer hash;
Review Comment:
Can u add some comments for this variable. Like explain why we introduce
this prefix.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
