Copilot commented on code in PR #11732:
URL: https://github.com/apache/gravitino/pull/11732#discussion_r3435784985
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/utils/DorisUtils.java:
##########
@@ -98,23 +101,30 @@ public static Map<String, String>
extractPropertiesFromSql(String createTableSql
public static Optional<Transform> extractPartitionInfoFromSql(String
createTableSql) {
try {
- String[] lines = createTableSql.split("\n");
- for (String line : lines) {
- Matcher matcher = PARTITION_INFO_PATTERN.matcher(line.trim());
- if (matcher.matches()) {
- String partitionType = matcher.group(1);
- String partitionInfoString = matcher.group(2);
- String[] columns =
- Arrays.stream(partitionInfoString.split(", "))
- .map(s -> s.substring(1, s.length() - 1))
- .toArray(String[]::new);
- if (LIST_PARTITION.equals(partitionType)) {
- String[][] filedNames =
- Arrays.stream(columns).map(s -> new String[]
{s}).toArray(String[][]::new);
- return Optional.of(Transforms.list(filedNames));
- } else if (RANGE_PARTITION.equals(partitionType)) {
- return Optional.of(Transforms.range(new String[] {columns[0]}));
+ // Merge all lines to handle multi-line partition definitions
+ String mergedSql = createTableSql.replaceAll("\\n", " ");
Review Comment:
`replaceAll("\\n", " ")` uses a regex engine unnecessarily. For large `SHOW
CREATE TABLE` outputs this adds avoidable overhead; a simple char replacement
is faster and avoids regex allocation.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/utils/DorisUtils.java:
##########
@@ -124,6 +134,107 @@ public static Optional<Transform>
extractPartitionInfoFromSql(String createTable
}
}
+ private static ListPartition[] extractListPartitionAssignments(String
mergedSql) {
+ try {
+ // Locate "PARTITION <name> VALUES IN (" and extract the outer paren
content manually
+ // to correctly handle multi-column partitions: VALUES IN (("a", 1),
("b", 2))
+ Pattern headerPattern =
+
Pattern.compile("PARTITION\\s+(?:`(\\w+)`|(\\w+))\\s+VALUES\\s+IN\\s*\\(");
Review Comment:
`headerPattern` only allows `\w+` inside backticks, so valid Doris
identifiers like `` `p-2024_07` `` (or any quoted name containing
`-`/`.`/spaces) will not be parsed and assignments will be silently skipped.
Since this change explicitly adds backtick support, the quoted branch should
accept any non-backtick content.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/utils/DorisUtils.java:
##########
@@ -98,23 +101,30 @@ public static Map<String, String>
extractPropertiesFromSql(String createTableSql
public static Optional<Transform> extractPartitionInfoFromSql(String
createTableSql) {
try {
- String[] lines = createTableSql.split("\n");
- for (String line : lines) {
- Matcher matcher = PARTITION_INFO_PATTERN.matcher(line.trim());
- if (matcher.matches()) {
- String partitionType = matcher.group(1);
- String partitionInfoString = matcher.group(2);
- String[] columns =
- Arrays.stream(partitionInfoString.split(", "))
- .map(s -> s.substring(1, s.length() - 1))
- .toArray(String[]::new);
- if (LIST_PARTITION.equals(partitionType)) {
- String[][] filedNames =
- Arrays.stream(columns).map(s -> new String[]
{s}).toArray(String[][]::new);
- return Optional.of(Transforms.list(filedNames));
- } else if (RANGE_PARTITION.equals(partitionType)) {
- return Optional.of(Transforms.range(new String[] {columns[0]}));
+ // Merge all lines to handle multi-line partition definitions
+ String mergedSql = createTableSql.replaceAll("\\n", " ");
+ Matcher matcher = PARTITION_INFO_PATTERN.matcher(mergedSql);
+ if (matcher.find()) {
+ String partitionType = matcher.group(1);
+ String partitionInfoString = matcher.group(2);
+ String[] columns =
+ Arrays.stream(partitionInfoString.split(","))
+ .map(String::trim)
+ .map(
+ s ->
+ (s.startsWith("`") && s.endsWith("`")) ?
s.substring(1, s.length() - 1) : s)
+ .toArray(String[]::new);
+ if (LIST_PARTITION.equals(partitionType)) {
+ String[][] filedNames =
+ Arrays.stream(columns).map(s -> new String[]
{s}).toArray(String[][]::new);
+ // Try to extract partition assignments
+ ListPartition[] assignments =
extractListPartitionAssignments(mergedSql);
+ if (assignments.length > 0) {
+ return Optional.of(Transforms.list(filedNames, assignments));
}
+ return Optional.of(Transforms.list(filedNames));
Review Comment:
Local variable `filedNames` is misspelled (should be `fieldNames`). Keeping
the typo makes the partition parsing code harder to read and search for.
##########
catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/utils/TestDorisUtils.java:
##########
@@ -100,42 +97,77 @@ public void testExtractPartitionInfoFromSql() {
String createTableSql =
"CREATE TABLE `testTable` (\n`col1` date NOT NULL\n) ENGINE=OLAP\n
PARTITION BY RANGE(`col1`)\n()\n DISTRIBUTED BY HASH(`col1`) BUCKETS 2";
Optional<Transform> transform =
DorisUtils.extractPartitionInfoFromSql(createTableSql);
- assertTrue(transform.isPresent());
- assertEquals(Transforms.range(new String[] {"col1"}), transform.get());
+ Assertions.assertTrue(transform.isPresent());
+ Assertions.assertEquals(Transforms.range(new String[] {"col1"}),
transform.get());
- // test list partition
+ // test list partition (no space between LIST and parenthesis)
createTableSql =
"CREATE TABLE `testTable` (\n`col1` int(11) NOT NULL\n) ENGINE=OLAP\n
PARTITION BY LIST(`col1`)\n()\n DISTRIBUTED BY HASH(`col1`) BUCKETS 2";
transform = DorisUtils.extractPartitionInfoFromSql(createTableSql);
- assertTrue(transform.isPresent());
- assertEquals(Transforms.list(new String[][] {{"col1"}}), transform.get());
+ Assertions.assertTrue(transform.isPresent());
+ Assertions.assertEquals(Transforms.list(new String[][] {{"col1"}}),
transform.get());
+
+ // test list partition with space (Doris 3.0+ format: "PARTITION BY LIST
(`col1`)")
+ createTableSql =
+ "CREATE TABLE `testTable` (\n`col1` int(11) NOT NULL\n) ENGINE=OLAP\n
PARTITION BY LIST (`col1`)\n()\n DISTRIBUTED BY HASH(`col1`) BUCKETS 2";
+ transform = DorisUtils.extractPartitionInfoFromSql(createTableSql);
+ Assertions.assertTrue(transform.isPresent());
+ Assertions.assertEquals(Transforms.list(new String[][] {{"col1"}}),
transform.get());
+
+ // test range partition with space (Doris 3.0+ may also add space for
RANGE)
+ createTableSql =
+ "CREATE TABLE `testTable` (\n`col1` date NOT NULL\n) ENGINE=OLAP\n
PARTITION BY RANGE (`col1`)\n()\n DISTRIBUTED BY HASH(`col1`) BUCKETS 2";
+ transform = DorisUtils.extractPartitionInfoFromSql(createTableSql);
+ Assertions.assertTrue(transform.isPresent());
+ Assertions.assertEquals(Transforms.range(new String[] {"col1"}),
transform.get());
// test multi-column list partition
createTableSql =
"CREATE TABLE `testTable` (\n`col1` date NOT NULL,\n`col2` int(11) NOT
NULL\n) ENGINE=OLAP\n PARTITION BY LIST(`col1`, `col2`)\n()\n DISTRIBUTED BY
HASH(`col1`) BUCKETS 2";
transform = DorisUtils.extractPartitionInfoFromSql(createTableSql);
- assertTrue(transform.isPresent());
- assertEquals(Transforms.list(new String[][] {{"col1"}, {"col2"}}),
transform.get());
+ Assertions.assertTrue(transform.isPresent());
+ Assertions.assertEquals(Transforms.list(new String[][] {{"col1"},
{"col2"}}), transform.get());
// test non-partitioned table
createTableSql =
"CREATE TABLE `testTable` (\n`testColumn` STRING NOT NULL COMMENT
'test comment'\n) ENGINE=OLAP\nCOMMENT \"test comment\"";
transform = DorisUtils.extractPartitionInfoFromSql(createTableSql);
- assertFalse(transform.isPresent());
+ Assertions.assertFalse(transform.isPresent());
+
+ // test multi-column list partition WITHOUT space after comma (C3 fix)
+ createTableSql =
+ "CREATE TABLE `testTable` (\n`col1` int(11) NOT NULL,\n`col2` int(11)
NOT NULL\n) ENGINE=OLAP\n PARTITION BY LIST(`col1`,`col2`)\n()\n DISTRIBUTED BY
HASH(`col1`) BUCKETS 2";
+ transform = DorisUtils.extractPartitionInfoFromSql(createTableSql);
+ Assertions.assertTrue(transform.isPresent());
+ Assertions.assertEquals(Transforms.list(new String[][] {{"col1"},
{"col2"}}), transform.get());
+
+ // test list partition with backtick-quoted partition names and
assignments (C4 fix)
+ createTableSql =
+ "CREATE TABLE `testTable` (\n`city` varchar(50) NOT NULL\n)
ENGINE=OLAP\n"
+ + " PARTITION BY LIST (`city`)\n"
+ + "(\n"
+ + " PARTITION `p1` VALUES IN (\"beijing\"),\n"
+ + " PARTITION `p2` VALUES IN (\"shanghai\")\n"
+ + ")\n"
+ + " DISTRIBUTED BY HASH(`city`) BUCKETS 1";
+ transform = DorisUtils.extractPartitionInfoFromSql(createTableSql);
+ Assertions.assertTrue(transform.isPresent());
+ Assertions.assertEquals("list", transform.get().name());
}
Review Comment:
The new test case for backtick-quoted partition names only asserts the
transform name, so it would still pass even if partition assignment parsing
(and name extraction) regresses. Since the PR adds logic to extract
assignments, assert the extracted partition names to lock the behavior down.
--
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]
