cloud-fan commented on code in PR #58648:
URL: https://github.com/apache/spark/pull/58648#discussion_r4068433115
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/FilterEstimation.scala:
##########
@@ -269,6 +280,59 @@ case class FilterEstimation(plan: Filter) extends Logging {
Some(percent)
}
+ /**
+ * Returns a percentage of rows meeting a null-intolerant string predicate
+ * (StartsWith / EndsWith / Contains) with a string literal operand.
+ *
+ * These predicates never match a null input, so at most the non-null rows
can match, giving an
+ * upper bound of `1 - nullPercent`. In addition, a value must have at least
as many characters
+ * as the operand, so if the operand is longer than the column's `maxLen` no
row can match. Both
+ * bounds reuse statistics already collected (`nullCount`, `maxLen`) and
hold under any collation
+ * (`maxLen` is a code-point count). This is a conservative estimate -- it
never under-estimates
+ * -- and improves on the default of 1.0 (all rows) used for unsupported
predicates.
+ *
+ * @param attr an Attribute (or a column)
+ * @param literal the non-null string literal operand
+ * @param update a boolean flag to specify if we need to update ColumnStat
of a given column
+ * for subsequent conditions
+ * @return an optional double value to show the percentage of rows meeting a
given condition.
+ * It returns None if no statistics are collected for a given column.
+ */
+ def evaluateStringPredicate(
+ attr: Attribute,
+ literal: Literal,
+ update: Boolean): Option[Double] = {
+ if (!colStatsMap.contains(attr) || colStatsMap(attr).nullCount.isEmpty) {
+ logDebug("[CBO] No statistics for " + attr)
+ return None
+ }
+ val colStat = colStatsMap(attr)
+ val rowCountValue = childStats.rowCount.get
+ val nullPercent: Double = if (rowCountValue == 0) {
+ 0
+ } else if (colStat.nullCount.get > rowCountValue) {
+ 1
+ } else {
+ (BigDecimal(colStat.nullCount.get) / BigDecimal(rowCountValue)).toDouble
+ }
+
+ // A value must have at least as many characters as the operand to start
with / end with /
+ // contain it. `maxLen` is the maximum code-point length, so this holds
under any collation.
+ val operandLength = literal.value.asInstanceOf[UTF8String].numChars()
+ val percent = if (colStat.maxLen.exists(_ < operandLength)) {
Review Comment:
**Non-blocking (P2):** After `IsNull(c)`, the mutable stats have
`distinctCount = 0` but retain the original `nullCount`. This helper then
recomputes the non-null fraction from the old row count and can retain rows for
`IsNull(c) AND StartsWith(c, ...)`, although null values cannot match;
reversing the conjunct order estimates zero. Please honor the all-null state
established by the earlier conjunct before deriving this bound.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/FilterEstimation.scala:
##########
@@ -269,6 +280,59 @@ case class FilterEstimation(plan: Filter) extends Logging {
Some(percent)
}
+ /**
+ * Returns a percentage of rows meeting a null-intolerant string predicate
+ * (StartsWith / EndsWith / Contains) with a string literal operand.
+ *
+ * These predicates never match a null input, so at most the non-null rows
can match, giving an
+ * upper bound of `1 - nullPercent`. In addition, a value must have at least
as many characters
+ * as the operand, so if the operand is longer than the column's `maxLen` no
row can match. Both
+ * bounds reuse statistics already collected (`nullCount`, `maxLen`) and
hold under any collation
+ * (`maxLen` is a code-point count). This is a conservative estimate -- it
never under-estimates
+ * -- and improves on the default of 1.0 (all rows) used for unsupported
predicates.
+ *
+ * @param attr an Attribute (or a column)
+ * @param literal the non-null string literal operand
+ * @param update a boolean flag to specify if we need to update ColumnStat
of a given column
+ * for subsequent conditions
+ * @return an optional double value to show the percentage of rows meeting a
given condition.
+ * It returns None if no statistics are collected for a given column.
+ */
+ def evaluateStringPredicate(
+ attr: Attribute,
+ literal: Literal,
+ update: Boolean): Option[Double] = {
+ if (!colStatsMap.contains(attr) || colStatsMap(attr).nullCount.isEmpty) {
Review Comment:
**Non-blocking (P2):** This path consumes `nullCount` even when the child is
not a leaf. The adjacent `IsNull`/`IsNotNull` handling deliberately refuses
that estimate because `JoinEstimation` does not maintain accurate null counts
for outer joins. A string predicate above such a join can therefore
under-estimate rows; please apply the same provenance restriction or use the
conservative fallback.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/FilterEstimation.scala:
##########
@@ -214,10 +215,20 @@ case class FilterEstimation(plan: Filter) extends Logging
{
case op @ GreaterThanOrEqual(attrLeft: Attribute, attrRight: Attribute)
=>
evaluateBinaryForTwoColumns(op, attrLeft, attrRight, update)
+ // StartsWith/EndsWith/Contains are null-intolerant, so only non-null
rows can match; and a
+ // value must be at least as long as the operand. We can bound their
selectivity from
+ // `nullCount` and `maxLen` even without distribution statistics. `Like`
still falls through
+ // (its common prefix/suffix/infix forms are already rewritten to the
operators below).
+ case StartsWith(ar: Attribute, l @ Literal(v, StringType)) if v != null
=>
Review Comment:
**Blocking (P1):** `evaluateStringPredicate` returns a conservative upper
bound, but the generic `Not` path complements every returned percentage as
though it were ordinary selectivity. For a non-null column this helper returns
1.0, so `NOT StartsWith(...)` is estimated at zero even when every row passes;
simplified `NOT LIKE 'A%'` can reach the same path. Please preserve the
upper-bound distinction or fall back conservatively when the result is negated.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/FilterEstimationSuite.scala:
##########
@@ -579,6 +579,54 @@ class FilterEstimationSuite extends
StatsEstimationTestBase {
expectedRowCount = 10)
}
+ test("cstring startsWith 'A' - bounded by the non-null fraction") {
+ // Only non-null rows can match, so selectivity <= 1 - nullPercent = 0.5.
+ val colStatNullableString = colStatString.copy(nullCount = Some(5))
+ validateEstimatedStats(
+ Filter(StartsWith(attrString, Literal("A")),
+ childStatsTestPlan(Seq(attrString), tableRowCount = 10L,
+ attributeMap = AttributeMap(Seq(attrString ->
colStatNullableString)))),
+ Seq(attrString -> colStatString.copy(distinctCount = Some(5))),
+ expectedRowCount = 5)
+ }
+
+ test("cstring endsWith / contains 'A' - bounded by the non-null fraction") {
+ val colStatNullableString = colStatString.copy(nullCount = Some(5))
+ Seq(EndsWith(attrString, Literal("A")), Contains(attrString,
Literal("A"))).foreach { cond =>
+ validateEstimatedStats(
+ Filter(cond,
+ childStatsTestPlan(Seq(attrString), tableRowCount = 10L,
+ attributeMap = AttributeMap(Seq(attrString ->
colStatNullableString)))),
+ Seq(attrString -> colStatString.copy(distinctCount = Some(5))),
+ expectedRowCount = 5)
+ }
+ }
+
+ test("cstring startsWith operand longer than maxLen matches nothing") {
+ // colStatString.maxLen is 2, so a 3-character prefix cannot match any
value.
+ validateEstimatedStats(
+ Filter(StartsWith(attrString, Literal("abc")),
childStatsTestPlan(Seq(attrString), 10L)),
+ Seq(attrString -> colStatString),
+ expectedRowCount = 0)
+ }
+
+ test("cstring startsWith on an all-null column matches nothing") {
+ val colStatAllNull = colStatString.copy(nullCount = Some(10))
+ validateEstimatedStats(
+ Filter(StartsWith(attrString, Literal("A")),
+ childStatsTestPlan(Seq(attrString), tableRowCount = 10L,
+ attributeMap = AttributeMap(Seq(attrString -> colStatAllNull)))),
+ Seq(attrString -> colStatAllNull),
+ expectedRowCount = 0)
+ }
+
+ test("cstring startsWith on a non-null column is unchanged (selectivity
1.0)") {
+ validateEstimatedStats(
Review Comment:
**Non-blocking (P2):** These positive leaf cases stay green for the three
failing interactions in the implementation: negating the new upper bound,
evaluating a string predicate after `IsNull` has established an all-null state,
and filtering above a non-leaf child with inaccurate `nullCount`. Please add
regression-sensitive cases for those paths so the conservative-cardinality
contract is enforced.
See **Shared repair plan 1** in the review body.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
