Re: [PR] Filter IS NOT NULL expression 's sizeInBytes should not increase over child [spark]

["cloud-fan (via GitHub)" <gi...@apache.org>]

cloud-fan commented on code in PR #55349:
URL: https://github.com/apache/spark/pull/55349#discussion_r3087038986


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/FilterEstimation.scala:
##########
@@ -47,17 +47,27 @@ case class FilterEstimation(plan: Filter) extends Logging {
 
     // Estimate selectivity of this filter predicate, and update column stats if needed.
     // For not-supported condition, set filter selectivity to a conservative estimate 100%
-    val filterSelectivity = calculateFilterSelectivity(plan.condition).getOrElse(1.0)
+    val filterSelectivity =
+      calculateFilterSelectivity(plan.condition).map(boundProbability).getOrElse(1.0)
 
-    val filteredRowCount: BigInt = ceil(BigDecimal(childStats.rowCount.get) * filterSelectivity)
+    val childRowCount = childStats.rowCount.get
+    val filteredRowCount: BigInt =
+      ceil(BigDecimal(childRowCount) * filterSelectivity).min(childRowCount)
     val newColStats = if (filteredRowCount == 0) {
       // The output is empty, we don't need to keep column stats.
       AttributeMap[ColumnStat](Nil)
     } else {
-      colStatsMap.outputColumnStats(rowsBeforeFilter = childStats.rowCount.get,
+      colStatsMap.outputColumnStats(rowsBeforeFilter = childRowCount,
         rowsAfterFilter = filteredRowCount)
     }
-    val filteredSizeInBytes: BigInt = getOutputSize(plan.output, filteredRowCount, newColStats)
+    val sizeByOutputAttrs = getOutputSize(plan.output, filteredRowCount, newColStats)
+    val sizeByChildScaling = if (childRowCount > 0 && filteredRowCount > 0) {
+      ceil(BigDecimal(childStats.sizeInBytes) * BigDecimal(filteredRowCount) / BigDecimal(childRowCount))
+        .max(BigInt(1))
+    } else {
+      BigInt(1)
+    }
+    val filteredSizeInBytes: BigInt = sizeByOutputAttrs.min(sizeByChildScaling)

Review Comment:
   The fix applies to all filter predicates (range filters, equality filters, etc.), not just IS NOT NULL — which is actually the right behavior. Consider updating the PR title/description to reflect the general scope, e.g., "Filter's sizeInBytes estimation should not inflate over child's sizeInBytes when CBO is on."



##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/FilterEstimationSuite.scala:
##########
@@ -290,6 +290,22 @@ class FilterEstimationSuite extends StatsEstimationTestBase {
       expectedRowCount = 10)
   }
 
+  test("IS NOT NULL should not increase sizeInBytes over child") {
+    val attrStringLarge = AttributeReference("cstring_large", StringType)()
+    val colStatStringLarge = ColumnStat(distinctCount = Some(10), min = None, max = None,
+      nullCount = Some(0), avgLen = Some(1000), maxLen = Some(1000))
+    val childPlan = StatsTestPlan(
+      outputList = Seq(attrStringLarge),
+      rowCount = 1000,
+      attributeStats = AttributeMap(Seq(attrStringLarge -> colStatStringLarge)),
+      size = Some(1000)
+    )
+    val filter = Filter(IsNotNull(attrStringLarge), childPlan)
+    val filterStats = filter.stats
+    assert(filterStats.rowCount.contains(1000))
+    assert(filterStats.sizeInBytes == 1000)

Review Comment:
   The test covers the IS NOT NULL scenario where no rows are filtered out. Consider adding a second test that exercises partial filtering with a realistic child size — e.g., a range filter `cint > 5` that keeps 5 out of 10 rows, with the child's `size = Some(100)`. This would demonstrate that proportional scaling works correctly for the general case: `sizeByChildScaling = ceil(100 * 5 / 10) = 50`.



-- 
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: reviews-unsubscribe@spark.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org