You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cravani (via GitHub)" <gi...@apache.org> on 2025/10/25 12:57:24 UTC
[PR] SPARK-54028: Use empty schema when altering a view which is not Hive compatible [spark]
cravani opened a new pull request, #52730:
URL: https://github.com/apache/spark/pull/52730
### What changes were proposed in this pull request?
Spark attempts to save views in a Hive-compatible format and only sets the schema to empty if the save operation fails.
However, due to certain Hive compatibility issues, the save operation may succeed while subsequent read operations fail. This issue arises after the change introduced in [SPARK-46934](https://issues.apache.org/jira/browse/SPARK-46934), which removed the verifyColumnDataType check during the ALTER TABLE operation.
### Why are the changes needed?
to not save malformed views that no one can read.
### Does this PR introduce _any_ user-facing change?
Yes, the malformed view will be saved in non hive compatible way so that Spark can read it.
### How was this patch tested?
Updated Test case
### Was this patch authored or co-authored using generative AI tooling?
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cravani (via GitHub)" <gi...@apache.org>.
cravani commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2480858211
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveMetastoreCatalogSuite.scala:
##########
@@ -438,6 +438,46 @@ class DataSourceWithHiveMetastoreCatalogSuite
}
}
+ test("SPARK-54028: Table and View with complex nested schema and ALTER operations") {
+ withTable("t") {
+ val schema =
+ "double_field DOUBLE," +
+ "id STRING," +
+ "integer_field BIGINT," +
+ "struct_field STRUCT<" +
+ "`colon:field_name`:STRING" +
Review Comment:
Working on updating the minimal repro for this test case...
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cravani (via GitHub)" <gi...@apache.org>.
cravani commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2478219202
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -598,7 +598,17 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
if (tableDefinition.tableType == VIEW) {
val newTableProps = tableDefinition.properties ++ tableMetaToTableProps(tableDefinition).toMap
- val newTable = tableDefinition.copy(properties = newTableProps)
+ val schemaWithNoCollation = removeCollation(tableDefinition.schema)
Review Comment:
The bug fix is similar to one addressed as part of SPARK-51594, I think we missed alter table part of it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #52730:
URL: https://github.com/apache/spark/pull/52730#issuecomment-3474531674
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2477610400
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveMetastoreCatalogSuite.scala:
##########
@@ -438,6 +438,46 @@ class DataSourceWithHiveMetastoreCatalogSuite
}
}
+ test("SPARK-54028: Table and View with complex nested schema and ALTER operations") {
+ withTable("t") {
+ val schema =
+ "double_field DOUBLE," +
+ "id STRING," +
+ "integer_field BIGINT," +
+ "struct_field STRUCT<" +
+ "`colon:field_name`:STRING" +
Review Comment:
seem we only need `struct_field` column to trigger this bug?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2477602071
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -598,7 +598,17 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
if (tableDefinition.tableType == VIEW) {
val newTableProps = tableDefinition.properties ++ tableMetaToTableProps(tableDefinition).toMap
- val newTable = tableDefinition.copy(properties = newTableProps)
+ val schemaWithNoCollation = removeCollation(tableDefinition.schema)
Review Comment:
Is this another bug fix?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2480765247
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -598,7 +598,17 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
if (tableDefinition.tableType == VIEW) {
val newTableProps = tableDefinition.properties ++ tableMetaToTableProps(tableDefinition).toMap
- val newTable = tableDefinition.copy(properties = newTableProps)
+ val schemaWithNoCollation = removeCollation(tableDefinition.schema)
+ val hiveCompatibleSchema = schemaWithNoCollation match {
+ // Spark-created views do not have to be Hive compatible. If the data type is not
+ // Hive compatible, we can set schema to empty so that Spark can still read this
+ // view as the schema is also encoded in the table properties.
+ case schema if
Review Comment:
we don't need a pattern match here. It's just
```
if (schemaWithNoCollation.exists...) {
EMPTY_DATA_SCHEMA
} else {
schemaWithNoCollation
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2477603606
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -598,7 +598,17 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
if (tableDefinition.tableType == VIEW) {
val newTableProps = tableDefinition.properties ++ tableMetaToTableProps(tableDefinition).toMap
- val newTable = tableDefinition.copy(properties = newTableProps)
+ val schemaWithNoCollation = removeCollation(tableDefinition.schema)
+ val hiveCompatibleSchema = schemaWithNoCollation match {
+ // Spark-created views do not have to be Hive compatible. If the data type is not
+ // Hive compatible, we can set schema to empty so that Spark can still read this
+ // view as the schema is also encoded in the table properties.
+ case schema if tableDefinition.tableType == CatalogTableType.VIEW &&
Review Comment:
```suggestion
case schema if
```
we are already inside `if (tableDefinition.tableType == VIEW)`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cravani (via GitHub)" <gi...@apache.org>.
cravani commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2480855948
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -598,7 +598,17 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
if (tableDefinition.tableType == VIEW) {
val newTableProps = tableDefinition.properties ++ tableMetaToTableProps(tableDefinition).toMap
- val newTable = tableDefinition.copy(properties = newTableProps)
+ val schemaWithNoCollation = removeCollation(tableDefinition.schema)
+ val hiveCompatibleSchema = schemaWithNoCollation match {
+ // Spark-created views do not have to be Hive compatible. If the data type is not
+ // Hive compatible, we can set schema to empty so that Spark can still read this
+ // view as the schema is also encoded in the table properties.
+ case schema if
Review Comment:
Working on updating the suggested changes...
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cravani (via GitHub)" <gi...@apache.org>.
cravani commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2478446450
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveMetastoreCatalogSuite.scala:
##########
@@ -438,6 +438,46 @@ class DataSourceWithHiveMetastoreCatalogSuite
}
}
+ test("SPARK-54028: Table and View with complex nested schema and ALTER operations") {
+ withTable("t") {
+ val schema =
+ "double_field DOUBLE," +
+ "id STRING," +
+ "integer_field BIGINT," +
+ "struct_field STRUCT<" +
+ "`colon:field_name`:STRING" +
Review Comment:
Yes that is correct we only need struct_field to reproduce the issue, I just added my complete test scenario. Let me know if existing test case is ok or I can modify to include struct_field alone.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52730:
URL: https://github.com/apache/spark/pull/52730#discussion_r2480767710
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveMetastoreCatalogSuite.scala:
##########
@@ -438,6 +438,46 @@ class DataSourceWithHiveMetastoreCatalogSuite
}
}
+ test("SPARK-54028: Table and View with complex nested schema and ALTER operations") {
+ withTable("t") {
+ val schema =
+ "double_field DOUBLE," +
+ "id STRING," +
+ "integer_field BIGINT," +
+ "struct_field STRUCT<" +
+ "`colon:field_name`:STRING" +
Review Comment:
test should be minimized to reproduce the bug, let's remove the unrelated columns.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #52730: [SPARK-54028][SQL] Use empty schema when altering a view which is not Hive compatible
URL: https://github.com/apache/spark/pull/52730
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org