LuciferYang commented on code in PR #60025:
URL: https://github.com/apache/spark/pull/60025#discussion_r4450653503
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/EstimationUtils.scala:
##########
@@ -87,6 +88,11 @@ 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.
+ case alias @ Alias(CharVarcharUtils.ReadSidePadding(attr: Attribute), _)
Review Comment:
Done in 51688d2ba39. The pass-through now raises `avgLen` and `maxLen` to at
least N, using the length argument the extractor returns, and
`ProjectEstimationSuite` covers stats collected over shorter values (`avgLen` 3
and `maxLen` 4 under `CHAR(5)` become 5 and 5). The distinct count is left as
is; it can only be too high when stored values differ just in trailing spaces.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/CharVarcharUtils.scala:
##########
@@ -391,6 +391,22 @@ object CharVarcharUtils extends Logging with
SparkCharVarcharUtils {
}.getOrElse(attr)
}
+ /**
+ * Matches the read-side CHAR padding that [[addPaddingForScan]] wraps a
column in (when
+ * standard semantics are off) and extracts the padded expression.
+ */
+ private[sql] object ReadSidePadding {
Review Comment:
I'd like to keep this PR to the default setup and handle standard semantics
in SPARK-60830. As you point out, the checks need a matcher of their own so
that `V2ExpressionBuilder` never compiles them as `CHAR_READ_SIDE_PADDING`.
They also need different length handling: a value from
`varcharTypeReadSideCheck` has at most N characters, so the length floor from
comment 1 does not apply there. The description now points at the follow-up.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/FilterEstimation.scala:
##########
@@ -170,7 +171,14 @@ case class FilterEstimation(
* It returns None if the condition is not supported.
*/
def calculateSingleCondition(condition: Expression, update: Boolean):
Option[Double] = {
- condition match {
+ // A predicate pushed below a read-side CHAR padding Project compares the
padding expression
+ // rather than the column. The column's statistics already describe padded
values (see
+ // `EstimationUtils.getAliasStats`), so estimate it as a predicate on the
column itself.
+ val unpadded = condition.mapChildren {
+ case CharVarcharUtils.ReadSidePadding(attr: Attribute) => attr
Review Comment:
Done in 51688d2ba39. The description now lists both callers, and
`StatisticsCollectionSuite` covers the partition pruning case: for a `CHAR(5)`
partition column with 10 values over 1000 rows, the relation pruned by `WHERE p
= '3'` gets a row count of 100. Without the change it stays at 1000.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/FilterEstimationSuite.scala:
##########
@@ -963,6 +964,26 @@ 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,
Review Comment:
Done in 51688d2ba39. Added `InSet` and `IsNotNull`. The `IsNotNull` case
uses a child with 4 nulls in 10 rows, because with no nulls the estimate is 10
rows, the same as for an unsupported predicate.
--
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]
