dongjoon-hyun commented on code in PR #57443:
URL: https://github.com/apache/spark/pull/57443#discussion_r3658919064


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/InMemoryRelation.scala:
##########
@@ -257,10 +265,14 @@ case class CachedRDDBuilder(
     storageLevel: StorageLevel,
     @transient cachedPlan: SparkPlan,
     tableName: Option[String],
-    @transient logicalPlan: LogicalPlan) {
+    @transient logicalPlan: LogicalPlan,
+    isCachedLogicalPlanRepeatable: Boolean = false,
+    hasSelectivePredicate: Boolean = false,
+    fileSourceOptions: Seq[Map[String, String]] = Seq.empty) {
 
   @transient @volatile private var _cachedColumnBuffers: RDD[CachedBatch] = 
null
-  @transient @volatile private var _cachedColumnBuffersAreLoaded: Boolean = 
false
+  @volatile private var isCachedRDDRepeatable = false
+  private var hasStrictFileSourceReads = true

Review Comment:
   `hasStrictFileSourceReads` seems never to recover once it becomes `false`. 
This starts as `true`, is only
   AND-ed down in `buildBuffers()`, and is not reset here, while the other 
per-generation states
   (`partitionStats`, `isCachedRDDRepeatable`) are. So if any earlier build of 
this builder observed a
   best-effort read (e.g. the session temporarily had 
`spark.sql.files.ignoreMissingFiles=true`), every
   later generation stays non-repeatable even when it is rebuilt with strict 
settings.
   
   This is safe (it only disables the optimization), but it seems inconsistent 
with the
   generation-isolation design of this PR — a rebuilt generation re-derives 
everything else from the
   configuration in effect at rebuild time. Was this intentional? If not, how 
about resetting it
   together with the other generation state:
   
   ```diff
        isCachedRDDRepeatable = false
   +    // Strictness is re-derived from the configuration in effect when the 
next generation is
   +    // built; a best-effort read observed by an earlier generation must not 
taint rebuilds.
   +    hasStrictFileSourceReads = true
      }
   ```
   
   If it is intentional (treating the cache as permanently tainted once any 
generation used
   best-effort reads), a short comment on the field explaining that would help.



-- 
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]

Reply via email to