cloud-fan commented on code in PR #58317:
URL: https://github.com/apache/spark/pull/58317#discussion_r3916512573
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala:
##########
@@ -752,15 +757,28 @@ case class FileSourceScanExec(
lazy val inputRDD: RDD[InternalRow] = {
val options = relation.options +
(FileFormat.OPTION_RETURNING_BATCH -> supportsColumnar.toString)
- val readFile: (PartitionedFile) => Iterator[InternalRow] =
- relation.fileFormat.buildReaderWithPartitionValues(
- sparkSession = relation.sparkSession,
- dataSchema = relation.dataSchema,
- partitionSchema = relation.partitionSchema,
- requiredSchema = requiredSchema,
- filters = pushedDownFilters,
- options = options,
- hadoopConf = getHadoopConf(relation.sparkSession, relation.options))
+ val hadoopConf = getHadoopConf(relation.sparkSession, relation.options)
+ val readFile: (PartitionedFile) => Iterator[InternalRow] =
relation.fileFormat match {
+ case format: OrcFileFormat if charVarcharStandardSemantics.isDefined =>
+ format.buildReaderWithPartitionValues(
Review Comment:
**Blocking (P1):** This type match also accepts external `OrcFileFormat`
subclasses, but the new package-private overload is not the public virtual
reader hook they can override. With either bound mode, their existing reader
adaptation is therefore skipped. Please keep the special overload limited to
the exact built-in format and route subclasses through
`buildReaderWithPartitionValues`, with a regression whose subclass override
must run under both bound modes.
**Recommended change:** Restrict the mode-aware private overload to the
exact built-in OrcFileFormat implementation and preserve public virtual
dispatch for every subclass.
**Why this works:** Make FileSourceScanExec distinguish the built-in
OrcFileFormat instance from derived formats; call the eight-argument
implementation only for the former and use the established seven-argument
override for the latter.
**Scope:** The V1 file-scan reader selection in DataSourceScanExec and a
custom OrcFileFormat subclass regression test.
**Compatibility:** Retain bound-mode schema selection for Spark's built-in
ORC source while restoring the extension contract used by existing
OrcFileFormat-derived connectors.
**Risks:** An imprecise exact-format check could still capture subclasses.
Built-in ORC behavior must remain covered for both Boolean modes and both
reader implementations.
**Constraints:** Do not require external subclasses to call or override a
private[sql] method. Do not reintroduce task-side SQLConf lookup for the
built-in ORC path.
**Success:** A custom subclass's public reader override is invoked with
Some(false) and Some(true), while the built-in V1 ORC row and vector readers
continue using the analyzed mode.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcUtils.scala:
##########
@@ -535,11 +548,14 @@ object OrcUtils extends Logging {
dataSchema: StructType,
resultSchema: StructType,
partitionSchema: StructType,
- conf: Configuration): String = {
+ conf: Configuration,
+ charVarcharStandardSemantics: Boolean): String = {
Review Comment:
**Nit (P3):** Please add an `@param charVarcharStandardSemantics` entry
explaining that `true` requests physical ORC STRING for Spark-side checks,
while `false` preserves native constrained types. This Boolean changes the
generated reader schema, but it is the only parameter omitted from the
structured Scaladoc.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -71,7 +71,7 @@ case class SaveIntoDataSourceCommand(
try {
val logicalRelation = LogicalRelation(relation,
toAttributes(relation.schema), None,
- false, None)
+ false, None, None)
Review Comment:
**Blocking (P1):** This recache probe has `charVarcharStandardSemantics =
None`, but an ordinary cached V1 relation is now analyzed as `Some(false)` or
`Some(true)`. Because `recacheByPlan` compares canonicalized plans with
`sameResult` without rebinding this probe, it selects no bound cache entry and
a successful append can leave stale rows visible. Please use a
mutation-specific match that invalidates every semantic variant of the returned
V1 relation without weakening normal cross-mode cache identity, and extend the
SPARK-39952 regression for both bound modes.
**Recommended change:** Add a V1 mutation-invalidation path that matches
cached LogicalRelations by the returned BaseRelation while deliberately
ignoring only the CHAR/VARCHAR result-semantics field.
**Why this works:** Have SaveIntoDataSourceCommand invoke a CacheManager
operation that finds every cached plan containing the written BaseRelation and
recaches those entries, rather than constructing one unbound semantic plan and
relying on sameResult.
**Scope:** SQL core cache invalidation in SaveIntoDataSourceCommand and
CacheManager, plus focused SaveIntoDataSourceCommandSuite coverage.
**Compatibility:** Preserve the new mode-sensitive sameResult behavior for
ordinary cache substitution while restoring write invalidation for None,
Some(false), and Some(true) cache entries.
**Risks:** A relation matcher that is broader than the written BaseRelation
could recache unrelated entries. Testing only one bound mode could leave the
symmetric mode mismatch undiscovered.
**Constraints:** Do not erase charVarcharStandardSemantics from ordinary
plan identity. Invalidate all cached descendants that read the mutated returned
BaseRelation.
**Success:** After successive V1 saves, materialized cached reads observe
the new rows under default, preserve-only, and standard semantics, while
preserve-only and standard read plans remain distinct for cache reuse.
--
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]
