Re: [PR] fix(spark): correct data-skipping translation for LIKE 'prefix%' (StartsWith) [hudi]

["wombatu-kun (via GitHub)" <gi...@apache.org>]

wombatu-kun commented on code in PR #19169:
URL: https://github.com/apache/hudi/pull/19169#discussion_r3695466566


##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/DataSkippingUtils.scala:
##########
@@ -318,16 +318,18 @@ object DataSkippingUtils extends Logging {
         })
 
       // Filter "colA like 'xxx%'"
-      // Translates to "colA_minValue <= xxx AND xxx <= colA_maxValue" for index lookup
+      // Translates to "colA_maxValue >= xxx AND (colA_minValue < xxx OR colA_minValue like 'xxx%')" for index lookup
       //
-      // NOTE: Since a) this operator matches strings by prefix and b) given that this column is going to be ordered
-      //       lexicographically, we essentially need to check that provided literal falls w/in min/max bounds of the
-      //       given column
+      // NOTE: Since a) this operator matches strings by prefix and b) given that this column is ordered
+      //       lexicographically, a file can contain a value carrying the prefix iff its [minValue, maxValue] range
+      //       overlaps the range of all strings with that prefix. Merely checking that the prefix falls within
+      //       [minValue, maxValue] is incorrect: a file whose values all start with the prefix has minValue > prefix
+      //       and would be wrongly pruned.
       case StartsWith(sourceExpr @ AllowedTransformationExpression(attrRef), v @ Literal(_: UTF8String, _)) =>
         getTargetIndexedColumnName(attrRef, indexedCols)
           .map { colName =>
             val targetExprBuilder: Expression => Expression = swapAttributeRefInExpr(sourceExpr, attrRef, _)
-            genColumnValuesEqualToExpression(colName, v, targetExprBuilder)
+            genColumnStartsWithExpression(colName, v, targetExprBuilder)

Review Comment:
   `release-0.15.0` carries the identical `StartsWith` translation through `genColumnValuesEqualToExpression`, so the defect looks long-standing rather than a 1.x regression - was the 0.15.x column in the issue's matrix traced to something other than this translation, such as column-stats index availability? If not, the description's regression framing understates which releases are affected.



##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestDataSkippingUtils.scala:
##########
@@ -176,9 +176,10 @@ object TestDataSkippingUtils {
         Seq(
           IndexRow("file_1", valueCount = 1, B_minValue = "aba", B_maxValue = "adf", B_nullCount = 1), // may contain strings starting w/ "abc"
           IndexRow("file_2", valueCount = 1, B_minValue = "adf", B_maxValue = "azy", B_nullCount = 0),
-          IndexRow("file_3", valueCount = 1, B_minValue = "aaa", B_maxValue = "aba", B_nullCount = 0)
+          IndexRow("file_3", valueCount = 1, B_minValue = "aaa", B_maxValue = "aba", B_nullCount = 0),
+          IndexRow("file_4", valueCount = 1, B_minValue = "abc123", B_maxValue = "abc345", B_nullCount = 0) // all strings start w/ "abc"; minValue > "abc"

Review Comment:
   The failure mode here is silently short query results, but the new coverage stops at the translated expression - nothing asserts that `LIKE 'prefix%'` returns the same rows with `hoodie.enable.data.skipping` on and off. `TestDataSkippingQuery` already has the cow/mor plus `initQueryIndexConf` scaffolding for that; a follow-up is fine if you want to keep this PR minimal.



##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestDataSkippingUtils.scala:
##########
@@ -176,9 +176,10 @@ object TestDataSkippingUtils {
         Seq(
           IndexRow("file_1", valueCount = 1, B_minValue = "aba", B_maxValue = "adf", B_nullCount = 1), // may contain strings starting w/ "abc"
           IndexRow("file_2", valueCount = 1, B_minValue = "adf", B_maxValue = "azy", B_nullCount = 0),
-          IndexRow("file_3", valueCount = 1, B_minValue = "aaa", B_maxValue = "aba", B_nullCount = 0)
+          IndexRow("file_3", valueCount = 1, B_minValue = "aaa", B_maxValue = "aba", B_nullCount = 0),
+          IndexRow("file_4", valueCount = 1, B_minValue = "abc123", B_maxValue = "abc345", B_nullCount = 0) // all strings start w/ "abc"; minValue > "abc"
         ),
-        Seq("file_1")),
+        Seq("file_1", "file_4")),

Review Comment:
   `genColumnStartsWithExpression` is only exercised with the identity `targetExprBuilder`, since `testStringsLookupFilterExpressionsSource` carries a composite case only under `Not`. Add a positive `lower(col("B")).startsWith("abc")` case over the uppercase rows so the transformed min/max path is covered too.



-- 
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: commits-unsubscribe@hudi.apache.org

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