spark-test commented on code in PR #60025:
URL: https://github.com/apache/spark/pull/60025#discussion_r4470151030
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/ProjectEstimationSuite.scala:
##########
@@ -163,6 +164,54 @@ class ProjectEstimationSuite extends
StatsEstimationTestBase {
assert(proj2.stats == expectedStats2)
}
+ test("SPARK-60810: column stats of a CHAR column survive read-side padding")
{
+ val c =
DataTypeUtils.toAttribute(CharVarcharUtils.replaceCharVarcharWithStringInSchema(
+ StructType(Seq(StructField("c", CharType(5))))).head)
+ val colStat = ColumnStat(distinctCount = Some(10), nullCount = Some(0),
+ avgLen = Some(5), maxLen = Some(5))
+ val child = StatsTestPlan(
+ outputList = Seq(c),
+ rowCount = 1000,
+ attributeStats = AttributeMap(Seq(c -> colStat)))
+
+ val padded = CharVarcharUtils.addPaddingForScan(c)
+ assert(padded != c, "expected a read-side padding expression for a CHAR
column")
+ val proj = Project(Seq(Alias(padded, "c")()), child)
+ assert(proj.stats.attributeStats.get(proj.output.head).contains(colStat))
+
+ // Statistics of values shorter than N, e.g. collected with read-side
padding off: every
+ // non-null padded value has at least N characters.
+ val shortStat = colStat.copy(avgLen = Some(3), maxLen = Some(4))
Review Comment:
**Nit (P3):** Every non-null case here reports lengths at or below N (5/5,
and 3/4 with and without a distinct count), so the test cannot tell
`stat.avgLen.map(_.max(n))` from an overwrite to `n`: replacing `max(n)` with
`_ => n` in `getAliasStats` keeps this test, the join test and the end-to-end
tests passing. The keep-larger half matters because `readSidePadding` returns
over-long values unchanged, so external tables or connector statistics can
report lengths above N. Such a regression would understate the row size of
padding Projects over those columns, and with it their `sizeInBytes` and
broadcast decisions, without any test failing. A case with lengths above N, for
example 7/9 under `CHAR(5)`, would pin it.
**Verification:**
- **Regression:** Check that reported lengths above N survive the padding
alias, so a length floor that overwrites lengths with N fails.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/EstimationUtils.scala:
##########
@@ -87,6 +88,26 @@ 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.
+ // A distinct count of 0 or a null count covering every row means no
non-null values, so
+ // there is nothing to pad (ANALYZE then stores the type's default size
as the lengths).
+ // The distinct count is left as is.
+ // It is too high only when stored values differ just in trailing
spaces, which the
+ // padding makes equal.
+ case alias @ Alias(CharVarcharUtils.ReadSidePadding(attr: Attribute,
Literal(n: Int, _)), _)
Review Comment:
**Blocking (P1):** This branch hands padded CHAR join keys their column
statistics, including a distinct count of 0: `ANALYZE TABLE` stores 0 for a
column that holds only NULLs, the branch passes such a stat through unchanged,
and the new `FilterEstimation` unwrap can also set it to 0 for an `IS NULL`
filter. `JoinEstimation.computeByNdv` divides by `max(leftNdv, rightNdv)` with
no zero check, and string-typed key pairs always reach it because
`ValueInterval` treats them as intersecting. With CBO on, a query like this one
over two analyzed tables whose `c CHAR(5)` holds only NULLs:
```sql
SELECT * FROM (SELECT a.id AS aid, b.id AS bid FROM t1 a FULL OUTER JOIN t2
b ON a.c = b.c) j
CROSS JOIN t3
```
now fails with `java.lang.ArithmeticException: Division by zero` when the
planner reads the full outer join's statistics. Before this PR the padded keys
had no statistics and the query planned.
This was reproduced by compiling this PR's `EstimationUtils.scala` and
`FilterEstimation.scala` against Spark 4.3.0, whose `JoinEstimation.scala` is
identical to the base branch: the query fails with them and plans without them,
and with 10 distinct values or with CBO off it plans in both runs. It also
fails under a parent join on another column (the full outer join becomes a left
outer join after filter inference, and `InjectRuntimeFilter` reads its
statistics), for an inner join with constraint propagation off, and with `IS
NULL` filters on both sides. A plain inner join is not affected, because its
inferred `IS NOT NULL` filters estimate both sides at 0 rows first. All-null
STRING keys already fail this way before this PR.
**Recommended change:** In JoinEstimation's distinct-count estimate, treat a
join key pair whose larger distinct count is 0 as matching no rows instead of
dividing by it, as join estimation already does for all-null numeric keys. Add
join estimation tests for padded CHAR and STRING keys whose statistics have a
distinct count of 0, and an end-to-end CBO test that plans a full outer join of
two analyzed all-null CHAR columns.
**Why this works:** A larger distinct count of 0 means neither key has a
non-null value, so the equality matches no rows. Returning 0 inner-joined rows
for that pair before the division gives the estimate that numeric keys without
min and max already get, because their pair counts as disjoint, and lets the
existing outer-join branches compute their outputs, such as the rows of both
sides for a full outer join.
**Scope:** A zero-distinct-count guard in JoinEstimation's distinct-count
join estimate, with join estimation and end-to-end CBO tests for all-null
padded CHAR keys.
**Compatibility:** Estimates for key pairs with a positive distinct count,
for numeric keys and for the histogram path are unchanged, and padded CHAR
columns keep the statistics this PR passes through.
**Risks:** The guard also changes all-null STRING and BINARY join keys,
which fail the same way in the base branch: their joins now plan with 0
matching rows. It is not behind a flag; turning CBO off is the only way to skip
it.
**Constraints:** Join estimates for key pairs with a positive distinct count
stay unchanged.
**Exclusions:**
- A key pair in which only one side has a distinct count of 0 keeps its
current estimate: it is an overestimate, not a failure.
**Success:** With CBO on, a full outer join of two analyzed CHAR(5) columns
that hold only NULLs plans under a cross join and under a join on another
column, and the full outer join is estimated at the rows of both sides. An
equi-join whose key pair has a distinct count of 0 on both sides is estimated
with 0 inner-joined rows instead of failing, for padded CHAR, STRING and BINARY
keys. Join estimates for key pairs with a positive distinct count are unchanged.
**Verification:**
- **Regression:** In the join estimation tests, join padded CHAR keys
through the read-side padding Project, a padded CHAR key with a STRING key, and
two STRING keys, all with a distinct count of 0 and a null count covering every
row, using inner and full outer joins, and check for 0 inner-joined rows (the
rows of both sides for the full outer join) instead of an ArithmeticException.
- **Regression:** With CBO on, analyze two CHAR(5) tables whose column holds
only NULLs and check that a full outer join on that column plans under a cross
join and under a join on another column, and that IS NULL filters on both sides
of the full outer join over columns with some NULLs plan too.
- **Behavior:** Check that the existing join estimation cases, including the
string, binary and padded CHAR keys with histograms, keep their estimates.
--
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]
