LuciferYang commented on code in PR #60025:
URL: https://github.com/apache/spark/pull/60025#discussion_r4464270762
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/FilterEstimation.scala:
##########
@@ -170,7 +173,11 @@ case class FilterEstimation(
* It returns None if the condition is not supported.
*/
def calculateSingleCondition(condition: Expression, update: Boolean):
Option[Double] = {
- condition match {
+ val unpadded = condition.mapChildren {
+ case CharVarcharUtils.ReadSidePadding(attr: Attribute, _) => attr
+ case other => other
+ }
+ unpadded match {
Review Comment:
Done in 31ac095029f and 1a264a7dee5. `evaluateEquality` now ignores a
histogram for string and binary columns and uses the distinct count, and
`JoinEstimation` takes the histogram path only when the intersected bounds
exist, which they never do for string and binary keys. This fixes STRING
columns as well. Since it is a few lines in the same estimators and this PR is
what sends CHAR columns there, I kept it here rather than in a separate JIRA.
Without it, the new tests fail with the `MatchError` you describe and with a
`NoSuchElementException` from the join path.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/EstimationUtils.scala:
##########
@@ -87,6 +88,15 @@ object EstimationUtils {
expressions.collect {
case alias @ Alias(attr: Attribute, _) if attributeStats.contains(attr)
=>
alias.toAttribute -> attributeStats(attr)
+ // Read-side CHAR padding wraps each CHAR column of a scan. With padding
on, ANALYZE TABLE
+ // reads the table through it as well, so the statistics already
describe padded values.
+ // Statistics collected otherwise (with padding off, or reported by a
connector) can
+ // describe shorter values, and every non-null padded value has at least
N characters.
+ case alias @ Alias(CharVarcharUtils.ReadSidePadding(attr: Attribute,
Literal(n: Int, _)), _)
+ if attributeStats.contains(attr) =>
+ val stat = attributeStats(attr)
+ alias.toAttribute -> stat.copy(
+ avgLen = stat.avgLen.map(_.max(n)), maxLen =
stat.maxLen.map(_.max(n)))
Review Comment:
Done in 31ac095029f and 1a264a7dee5. The floor is skipped when the distinct
count is 0, and `ProjectEstimationSuite` covers an all-null `CHAR(30)` column,
whose lengths stay at 20. A stat without a distinct count still gets the floor.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/EstimationUtils.scala:
##########
@@ -87,6 +88,15 @@ object EstimationUtils {
expressions.collect {
case alias @ Alias(attr: Attribute, _) if attributeStats.contains(attr)
=>
alias.toAttribute -> attributeStats(attr)
+ // Read-side CHAR padding wraps each CHAR column of a scan. With padding
on, ANALYZE TABLE
+ // reads the table through it as well, so the statistics already
describe padded values.
+ // Statistics collected otherwise (with padding off, or reported by a
connector) can
+ // describe shorter values, and every non-null padded value has at least
N characters.
Review Comment:
Done in 31ac095029f, with your wording plus a sentence on the distinct count
of 0 from comment 9.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/FilterEstimationSuite.scala:
##########
@@ -963,6 +965,31 @@ class FilterEstimationSuite extends
StatsEstimationTestBase {
expectedRowCount = 10)
}
+ test("SPARK-60810: predicates on a read-side padded CHAR column use the
column's stats") {
+ val padded = CharVarcharUtils.addPaddingForScan(
+
DataTypeUtils.toAttribute(CharVarcharUtils.replaceCharVarcharWithStringInSchema(
+ StructType(Seq(StructField("cstring",
CharType(2))))).head).withExprId(attrString.exprId))
+ assert(padded match {
+ case CharVarcharUtils.ReadSidePadding(a: Attribute, _) => a.exprId ==
attrString.exprId
+ case _ => false
+ }, s"expected read-side padding over cstring, got $padded")
+ val child = childStatsTestPlan(Seq(attrString), tableRowCount = 10L)
+ // colStatString has 10 distinct values and no nulls over 10 rows.
+ Seq(
+ EqualTo(padded, Literal("A0")) -> 1,
+ EqualTo(Literal("A0"), padded) -> 1,
+ In(padded, Seq(Literal("A0"), Literal("A1"))) -> 2,
+ InSet(padded, Set(UTF8String.fromString("A0"),
UTF8String.fromString("A1"))) -> 2,
+ IsNull(padded) -> 0).foreach { case (condition, expectedRows) =>
+ val filter = Filter(condition, child)
+ assert(filter.stats.rowCount.contains(BigInt(expectedRows)), condition)
Review Comment:
Done in 31ac095029f. The equality case cannot tell the update from plain
scaling, since both leave a distinct count of 1, so the test checks the output
stat of `IS NOT NULL` over a child with 4 nulls instead: the update sets
`nullCount` to 0, where scaling alone would give 3. I also added the
`Not(EqualTo(...))` case.
--
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]
