szehon-ho commented on code in PR #58851:
URL: https://github.com/apache/spark/pull/58851#discussion_r4031041084
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/PartitionPredicateImpl.scala:
##########
@@ -58,7 +78,8 @@ class PartitionPredicateImpl private (
try {
boundPredicate(partitionValues)
} catch {
- case e: Exception =>
+ // Propagated when this predicate is the only evaluator: see `failOpen`.
Review Comment:
nit: not sure what do you mean 'is the only evaluator'. do you mean :
```// When evaluation is pruning-only, keep the partition on failure;
// otherwise, let the exception propagate.```
if we rename the variable as i suggest, i think its more clear and we may
not need comment?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/PartitionPredicateImpl.scala:
##########
@@ -26,10 +27,23 @@ import
org.apache.spark.sql.connector.expressions.filter.PartitionPredicate
/**
* An implementation for [[PartitionPredicate]] that wraps a Catalyst
Expression representing a
* partition filter.
+ *
+ * @param catalystExpr the partition filter this predicate evaluates.
+ * @param partitionFields one entry per transform of `Table.partitioning()`,
in that order, so a
+ * bound ordinal matches the partition key a connector
passes to [[eval]].
+ * @param failOpen what [[eval]] does when it cannot evaluate the expression
for a partition.
Review Comment:
can we rename it for clarity? maybe 'keepPartitionOnEvaluationFailure'
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/PartitionPredicateImpl.scala:
##########
@@ -47,6 +61,12 @@ class PartitionPredicateImpl private (
override def eval(partitionValues: InternalRow): Boolean = {
if (partitionValues.numFields != partitionFields.length) {
+ if (!failOpen) {
+ throw SparkException.internalError(
Review Comment:
can we make the message once? like for example
```
val message =
log"Cannot evaluate partition predicate ${MDC(LogKeys.EXPR,
catalystExpr.sql)}: " +
log"partition value field count " +
log"(${MDC(LogKeys.COUNT, partitionValues.numFields)}) does not match
schema " +
log"(${MDC(LogKeys.NUM_PARTITIONS, partitionFields.length)})."
if (!keepPartitionOnEvaluationFailure) {
throw SparkException.internalError(message.message)
}
logWarning(message + log" Including partition in scan result.")
return true
```
--
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]
