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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
