yihua commented on code in PR #13523:
URL: https://github.com/apache/hudi/pull/13523#discussion_r2229422247
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/feature/index/testHoodieBackedTableMetadataIndexLookup.scala:
##########
@@ -214,13 +214,13 @@ abstract class
HoodieBackedTableMetadataIndexLookupTestBase extends HoodieSparkS
assert(emptyResult.collectAsList().isEmpty, "Empty input should return
empty result")
// Case 2: All existing keys
- val allKeys = HoodieListData.eager(List("1", "2", "$").asJava)
+ val allKeys = HoodieListData.eager(List("a1", "a2", "a$").asJava)
Review Comment:
Similarly, test with `$a`, `a$a` as well
##########
hudi-common/src/test/java/org/apache/hudi/metadata/TestSecondaryIndexKeyUtils.java:
##########
@@ -135,4 +136,74 @@ public void testBoundaryConditions() {
extractedRecordKey =
SecondaryIndexKeyUtils.getRecordKeyFromSecondaryIndexKey(constructedKey);
assertEquals(onlySpecialChars, extractedRecordKey);
}
+
+ // Test constructSecondaryIndexKeyPrefix method
Review Comment:
This unit test looks redundant as `constructSecondaryIndexKeyPrefix` is
pretty straighforward. Shall we remove it?
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/feature/index/testHoodieBackedTableMetadataIndexLookup.scala:
##########
@@ -157,9 +157,9 @@ abstract class HoodieBackedTableMetadataIndexLookupTestBase
extends HoodieSparkS
// Secondary index is created by default for non record key column when
index type is not specified
testData = Seq(
- Seq("b1", "b1", 10.0, 1000),
- Seq("b2", "b2", 20.0, 1000),
- Seq("$", "$", 30.0, 1000)
+ Seq("a1", "b1", 10.0, 1000),
+ Seq("a2", "b2", 20.0, 1000),
+ Seq("a$", "b$", 30.0, 1000)
Review Comment:
Similar for secondary key values
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/feature/index/testHoodieBackedTableMetadataIndexLookup.scala:
##########
@@ -157,9 +157,9 @@ abstract class HoodieBackedTableMetadataIndexLookupTestBase
extends HoodieSparkS
// Secondary index is created by default for non record key column when
index type is not specified
testData = Seq(
- Seq("b1", "b1", 10.0, 1000),
- Seq("b2", "b2", 20.0, 1000),
- Seq("$", "$", 30.0, 1000)
+ Seq("a1", "b1", 10.0, 1000),
+ Seq("a2", "b2", 20.0, 1000),
+ Seq("a$", "b$", 30.0, 1000)
Review Comment:
Could we add record keys where the `$` is at the beginning or middle of the
String, e.g., `$a`, `a$a`?
--
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]
