hudi-agent commented on code in PR #19536:
URL: https://github.com/apache/hudi/pull/19536#discussion_r3723504752
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -174,7 +174,11 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
} getOrElse {
Try(schemaResolver.getTableSchema) match {
case Success(schema) => schema
- case Failure(e) => throw e
+ case Failure(_) =>
+ // Schema not found on the dataset, so fetching schema from HMS.
+ logWarning(s"Schema not found on the dataset for $tableName, so
fetching schema from HMS.")
+ val catalogTable =
sparkSession.sessionState.catalog.externalCatalog.getTable(metaClient.getDbName,
metaClient.getTableName)
+ convertToHoodieSchema(catalogTable.schema, tableName)
Review Comment:
🤖 The normal getTableSchema() path includes the _hoodie_* meta fields (when
populateMetaFields is true). Spark-SQL-created tables do persist meta fields in
the HMS schema, so this works for the added test, but tables registered via
Hive sync / DataSource writer may store the catalog schema without meta fields.
Have you checked that catalogTable.schema here always includes the meta fields,
so tableStructSchema stays consistent with the non-fallback path?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -174,7 +174,11 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
} getOrElse {
Try(schemaResolver.getTableSchema) match {
case Success(schema) => schema
- case Failure(e) => throw e
+ case Failure(_) =>
+ // Schema not found on the dataset, so fetching schema from HMS.
+ logWarning(s"Schema not found on the dataset for $tableName, so
fetching schema from HMS.")
+ val catalogTable =
sparkSession.sessionState.catalog.externalCatalog.getTable(metaClient.getDbName,
metaClient.getTableName)
Review Comment:
🤖 For path-based reads (or tables not registered in the current session
catalog), metaClient.getDbName / getTableName may not correspond to an HMS
entry, and externalCatalog.getTable(...) will throw NoSuchDatabase/NoSuchTable,
replacing the original schema error with a more confusing one. Is the fallback
guaranteed to run only when the table is HMS-registered? It may be worth
catching the not-found case and rethrowing the original resolver failure.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -174,7 +174,11 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
} getOrElse {
Try(schemaResolver.getTableSchema) match {
case Success(schema) => schema
- case Failure(e) => throw e
+ case Failure(_) =>
+ // Schema not found on the dataset, so fetching schema from HMS.
+ logWarning(s"Schema not found on the dataset for $tableName, so
fetching schema from HMS.")
Review Comment:
🤖 nit: the inline comment repeats the log message verbatim — could you drop
one of them? The `logWarning` already serves as self-documentation here.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -174,7 +174,11 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
} getOrElse {
Try(schemaResolver.getTableSchema) match {
case Success(schema) => schema
- case Failure(e) => throw e
+ case Failure(_) =>
Review Comment:
🤖 This falls back to HMS on any getTableSchema failure, not just the
intended empty/no-data case. If the table actually has data but resolution
fails for another reason (transient IO, corruption, or a schema evolved after
create), we'd silently read with the possibly-stale HMS create-schema instead
of failing loudly. Could we gate the fallback on the table genuinely having no
resolvable schema (e.g. no completed commits / no base files), and chain the
original exception into anything we rethrow so the root cause isn't lost?
<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]
