Copilot commented on code in PR #13997: URL: https://github.com/apache/gravitino/pull/13997#discussion_r4480803549
########## catalogs/hive-metastore-common/src/test/java/org/apache/gravitino/hive/TestHivePartition.java: ########## @@ -0,0 +1,116 @@ +/* + * 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.gravitino.hive; + +import com.google.common.collect.ImmutableList; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import org.apache.gravitino.rel.expressions.literals.Literal; +import org.apache.gravitino.rel.expressions.literals.Literals; +import org.apache.hadoop.hive.common.FileUtils; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** Tests for {@link HivePartition} name escaping against HMS conventions. */ +public class TestHivePartition { + + private static final List<String> SPECIAL_VALUES = + ImmutableList.of("a/b", "x=y", "100%", "a b", "plain"); + + /** + * Names built from values must match what HMS (and the converter read path via + * FileUtils.makePartName) produces, and identity(name) must round-trip the raw values back. On + * the unfixed tree the plain concatenation produced names that HMS parses into different values + * (e.g. "ds=a/b" splits into two), breaking get/drop lookups. + */ + @Test + public void testPartitionNamesEscapeSpecialCharacters() { + String[][] fieldNames = {{"ds"}}; + for (String value : SPECIAL_VALUES) { + Literal<?>[] values = {Literals.stringLiteral(value)}; + HivePartition partition = HivePartition.identity(fieldNames, values); + + String expected = FileUtils.makePartName(ImmutableList.of("ds"), ImmutableList.of(value)); + Assertions.assertEquals(expected, partition.name(), "value: " + value); + + HivePartition parsed = HivePartition.identity(partition.name()); + Assertions.assertEquals(value, parsed.values()[0].value(), "value: " + value); + Assertions.assertEquals("ds", parsed.fieldNames()[0][0]); + } + } + + /** + * The invariant behind both directions: buildPartitionName equals makePartName for the same + * inputs, including multi-key partitions. + */ + @Test + public void testMultiKeyPartitionNamesMatchHmsConvention() { + String[][] fieldNames = {{"ds"}, {"region"}}; + List<String> rawValues = SPECIAL_VALUES.subList(0, 2); + Literal<?>[] values = rawValues.stream().map(Literals::stringLiteral).toArray(Literal[]::new); + HivePartition partition = HivePartition.identity(fieldNames, values); + + Assertions.assertEquals( + FileUtils.makePartName(ImmutableList.of("ds", "region"), rawValues), partition.name()); + + List<String> parsed = + IntStream.range(0, parsedValues(partition).size()) + .mapToObj(i -> parsedValues(partition).get(i)) Review Comment: `parsedValues(partition)` is recomputed multiple times inside the stream pipeline, which is unnecessary work and makes the test harder to read. Store the list once (e.g., `List<String> parsedValues = parsedValues(partition);`) and iterate over that. ########## catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/HivePartition.java: ########## @@ -187,7 +207,44 @@ public static String[][] extractPartitionFieldNames(String partitionName) { return new String[0][0]; } return Arrays.stream(partitionName.split(PARTITION_NAME_DELIMITER)) - .map(part -> new String[] {part.split(PARTITION_VALUE_DELIMITER, 2)[0]}) + .map(part -> new String[] {unescapePathName(part.split(PARTITION_VALUE_DELIMITER, 2)[0])}) .toArray(String[][]::new); } + + /** Escapes a partition column name or value the way Hive's FileUtils.escapePathName does. */ + static String escapePathName(String path) { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < path.length(); i++) { + char c = path.charAt(i); + if (c < CHARS_TO_ESCAPE.size() && CHARS_TO_ESCAPE.get(c)) { + sb.append('%').append(String.format("%1$02X", (int) c)); + } else { + sb.append(c); + } + } + return sb.toString(); + } Review Comment: `String.format` inside the per-character loop is relatively expensive and can become a hotspot when constructing many partition names. Consider replacing it with a lightweight hex append (e.g., append('%') then append two hex digits via a small lookup table) and optionally initialize `StringBuilder` with `path.length()` capacity. ########## catalogs/hive-metastore-common/src/test/java/org/apache/gravitino/hive/TestHivePartition.java: ########## @@ -0,0 +1,116 @@ +/* + * 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.gravitino.hive; + +import com.google.common.collect.ImmutableList; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import org.apache.gravitino.rel.expressions.literals.Literal; +import org.apache.gravitino.rel.expressions.literals.Literals; +import org.apache.hadoop.hive.common.FileUtils; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** Tests for {@link HivePartition} name escaping against HMS conventions. */ +public class TestHivePartition { + + private static final List<String> SPECIAL_VALUES = + ImmutableList.of("a/b", "x=y", "100%", "a b", "plain"); + + /** + * Names built from values must match what HMS (and the converter read path via + * FileUtils.makePartName) produces, and identity(name) must round-trip the raw values back. On + * the unfixed tree the plain concatenation produced names that HMS parses into different values + * (e.g. "ds=a/b" splits into two), breaking get/drop lookups. + */ + @Test + public void testPartitionNamesEscapeSpecialCharacters() { + String[][] fieldNames = {{"ds"}}; + for (String value : SPECIAL_VALUES) { + Literal<?>[] values = {Literals.stringLiteral(value)}; + HivePartition partition = HivePartition.identity(fieldNames, values); + + String expected = FileUtils.makePartName(ImmutableList.of("ds"), ImmutableList.of(value)); + Assertions.assertEquals(expected, partition.name(), "value: " + value); + + HivePartition parsed = HivePartition.identity(partition.name()); + Assertions.assertEquals(value, parsed.values()[0].value(), "value: " + value); + Assertions.assertEquals("ds", parsed.fieldNames()[0][0]); + } + } + + /** + * The invariant behind both directions: buildPartitionName equals makePartName for the same + * inputs, including multi-key partitions. + */ + @Test + public void testMultiKeyPartitionNamesMatchHmsConvention() { + String[][] fieldNames = {{"ds"}, {"region"}}; + List<String> rawValues = SPECIAL_VALUES.subList(0, 2); + Literal<?>[] values = rawValues.stream().map(Literals::stringLiteral).toArray(Literal[]::new); + HivePartition partition = HivePartition.identity(fieldNames, values); + + Assertions.assertEquals( + FileUtils.makePartName(ImmutableList.of("ds", "region"), rawValues), partition.name()); + + List<String> parsed = + IntStream.range(0, parsedValues(partition).size()) + .mapToObj(i -> parsedValues(partition).get(i)) + .collect(Collectors.toList()); + Assertions.assertEquals(rawValues, parsed); + } + + /** + * The two-arg extract (drop's partial-value filter) must unescape HMS-canonical names too, or a + * drop of ds=a%2Fb filters against the escaped string and never matches the stored raw a/b. + */ + @Test + public void testDropPathFilterUnescapesCanonicalNames() { + HivePartition partition = + HivePartition.identity( + new String[][] {{"ds"}}, new Literal[] {Literals.stringLiteral("a/b")}); + List<String> filter = + HivePartition.extractPartitionValues(ImmutableList.of("ds"), partition.name()); + Assertions.assertEquals(ImmutableList.of("a/b"), filter); + } + + /** The copied escape rule must match Hive's for every character. */ + @Test + public void testEscapingMatchesHiveFileUtils() { + for (char c = 0; c < Character.MAX_VALUE; c++) { + String value = "a" + c + "b"; + String escaped = HivePartition.escapePathName(value); + Assertions.assertEquals(FileUtils.escapePathName(value), escaped, "char: " + (int) c); + Assertions.assertEquals(value, HivePartition.unescapePathName(escaped), "char: " + (int) c); + } Review Comment: This test performs ~65k iterations and calls into Hive `FileUtils` each time, which can noticeably slow CI in some environments. If runtime becomes an issue, consider reducing the tested character set (while still covering all escaped chars + boundary/control cases) or marking this as a slower/integration-style test if your test framework supports it. ########## spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/utils/HiveGravitinoOperationOperator.java: ########## @@ -145,9 +146,11 @@ private InternalRow toSparkPartition(String partitionName, StructType partitionS String[] splits = partitionName.split(PARTITION_NAME_DELIMITER); Object[] values = new Object[splits.length]; for (int i = 0; i < splits.length; i++) { + // Partition names use the Hive path escaping, which Gravitino and HMS share. values[i] = SparkPartitionUtils.getSparkPartitionValue( - splits[i].split(PARTITION_VALUE_DELIMITER)[1], partitionSchema.apply(i).dataType()); + ExternalCatalogUtils.unescapePathName(splits[i].split(PARTITION_VALUE_DELIMITER)[1]), Review Comment: Using `split(PARTITION_VALUE_DELIMITER)` without a limit creates unnecessary intermediate strings/arrays and is less robust than splitting into at most 2 parts. Consider `split(PARTITION_VALUE_DELIMITER, 2)` here to reduce allocations and avoid surprises if an unexpected delimiter appears. -- 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]
