hudi-agent commented on code in PR #20120:
URL: https://github.com/apache/hudi/pull/20120#discussion_r4127103852
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala:
##########
@@ -971,6 +972,26 @@ class HoodieSparkSqlWriterInternal {
}
}
+ // Spark's own file-based writers invalidate the session cache from
+ // InsertIntoHadoopFsRelationCommand. Hudi writes do not go through that
command, so without
+ // this a cached Hudi table keeps serving the pre-write snapshot with no
signal to the reader.
+ // Matching by path rather than by plan also reaches entries built from a
DataFrame that was
+ // never registered in the catalog, which the refreshTable below cannot
see.
+ //
+ // A failure here must not fail the write. The commit has already
succeeded, and
+ // CacheManager.recacheByCondition drops the matching entries before it
attempts to rebuild
+ // them, so the invalidation has taken effect even when the rebuild
throws. The rebuild does
+ // throw when the cached plan is no longer valid against the table it was
built from: an
+ // overwrite that replaces a partitioned table with a non-partitioned one
leaves the cached
+ // plan holding the old partition schema, and re-optimizing it fails on
the new layout.
+ try {
+ spark.catalog.refreshByPath(basePath.toString)
Review Comment:
🤖 Confirmed at the PR head. When any sync tool fails, `metaSync` throws
`getHoodieMetaSyncException` at line 971, so it never reaches `refreshByPath`,
even though `client.commit` has already succeeded. The same applies to the
bootstrap caller at line 823. One alternative to moving the block up: wrap the
sync loop in `try { ... } finally { refreshByPath }`. Invalidation would then
run on both the success and failure paths, and it would still run after sync.
That ordering matters if a cached plan is built on the synced catalog table,
because its schema may only be updated by the sync.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala:
##########
@@ -971,6 +972,26 @@ class HoodieSparkSqlWriterInternal {
}
}
+ // Spark's own file-based writers invalidate the session cache from
+ // InsertIntoHadoopFsRelationCommand. Hudi writes do not go through that
command, so without
+ // this a cached Hudi table keeps serving the pre-write snapshot with no
signal to the reader.
+ // Matching by path rather than by plan also reaches entries built from a
DataFrame that was
+ // never registered in the catalog, which the refreshTable below cannot
see.
+ //
+ // A failure here must not fail the write. The commit has already
succeeded, and
+ // CacheManager.recacheByCondition drops the matching entries before it
attempts to rebuild
+ // them, so the invalidation has taken effect even when the rebuild
throws. The rebuild does
+ // throw when the cached plan is no longer valid against the table it was
built from: an
+ // overwrite that replaces a partitioned table with a non-partitioned one
leaves the cached
+ // plan holding the old partition schema, and re-optimizing it fails on
the new layout.
+ try {
Review Comment:
🤖 Is the claim that all matching entries' blocks are cleared before the
rebuild accurate? My reading of `recacheByCondition` is that it clears and
rebuilds one entry at a time. If one rebuild throws, the entries after it are
removed from `cachedData` but never unpersisted, so their blocks stay in memory
until GC cleans them up. Could you tighten the comment, or say this is
best-effort?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
