sunchao commented on code in PR #59269:
URL: https://github.com/apache/spark/pull/59269#discussion_r4209561523
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileSourceStrategy.scala:
##########
@@ -418,11 +441,19 @@ object FileSourceStrategy extends Strategy with
PredicateHelper with Logging {
Alias(KnownNotNull(CreateStruct(structColumns.toImmutableArraySeq)),
FileFormat.METADATA_NAME)(exprId = metadataStruct.exprId)
execution.ProjectExec(
- readDataColumns ++ partitionColumns :+ metadataAlias, scan)
+ readDataColumns ++ storageFilterCheckedColumn ++ partitionColumns :+
metadataAlias, scan)
}.getOrElse(scan)
+ // The offered conjuncts are skipped on the rows the reader marks as
checked. They are
+ // matched by `ExpressionSet`, since `storageFilters` carry the
relation's column names.
+ val postScanFilters = storageFilterCheckedColumn match {
+ case Some(checked) =>
+ val offered = ExpressionSet(storageFilters)
+ afterScanFilters.toSeq.map(f => if (offered.contains(f)) Or(checked,
f) else f)
Review Comment:
[P2] Normalize the nullable marker before composing it with other conjuncts
The supported parquet-mr fallback returns null for this missing column. With
whole-stage codegen disabled, `FilterEvaluatorFactory` evaluates the entire
combined `And` condition. For a rejecting bloom followed by `CAST(v AS INT) >
0`, the original `false AND rhs` skips the cast, but this rewrite produces
`(NULL OR false) AND rhs`: `And` evaluates its RHS on null and throws for `v =
'bad'`. Whole-stage filtering can conceal this because it checks conjuncts
separately.
Please make the marker non-null before OR, e.g. `Or(Coalesce(Seq(checked,
Literal(false))), f)`, and extend the existing non-whole-stage parquet-mr
fallback test with a rejecting bloom followed by a throwing conjunct. This
preserves the original conjunct's value when the row was not checked.
A focused component probe reproduced the error in interpreted,
generated-predicate, and `FilterEvaluatorFactory` execution, with both the
original condition and the coalesced-marker version correctly rejecting the
row. It compiled those relevant definitions from this head against cached Spark
4.1.3 dependencies; the full PR suite was blocked at build loading.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1982,13 +1982,16 @@ object SQLConf {
"only for the rows that survived. This is a planning-time decision. " +
"A filter that is attached also stays in the post-scan filter, the way
a pushed data " +
"filter does, so the reader is free to stop applying it wherever doing
so would cost " +
- "more than it saves, and the answer does not change. A row group where
it stops reads " +
+ "more than it saves, and the answer does not change. The post-scan
filter skips it on " +
+ "the rows the reader applied it to, except where one more column would
push the scan " +
+ "past spark.sql.codegen.maxFields. A row group where it stops reads " +
Review Comment:
Please document the reserved physical-column name in this configuration
contract.
The planner comment and PR body declare a file containing
`_tmp_storage_filter_checked` outside the relation schema unsupported, but this
user-facing documentation omits the restriction. An explicit/catalog schema can
hide that physical field from the relation-schema guard. If the read then falls
back to parquet-mr, a physical Boolean true can become the marker and bypass
the residual predicate.
This is an acknowledged compatibility restriction, rather than an additional
claim about supported-input correctness. Please expose it here and in the
user-facing-change section. An explicit footer-collision diagnostic or a
fallback that always synthesizes the marker would also make the unsupported
case safer to reason about. This consequence is source-derived; I did not
execute a physical-collision reproduction.
--
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]
