Skip to content

[SPARK-55645][SQL][FOLLOWUP] Move serdeName to last parameter and filter empty strings #54860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cloud-fan wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[SPARK-55645][SQL][FOLLOWUP] Move serdeName to last parameter and filter empty strings #54860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b302e9
[SPARK-55645][SQL][FOLLOWUP] Move serdeName to last parameter and fil…
cloud-fan Mar 17, 2026
d8e2539
Add test for empty serdeName filtering
cloud-fan Mar 17, 2026
3459244
Apply suggestions from code review
cloud-fan Mar 18, 2026
9cf3bb0
Update HiveClientImpl.scala
cloud-fan Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,10 @@ case class CatalogStorageFormat(
locationUri: Option[URI],
inputFormat: Option[String],
outputFormat: Option[String],
serdeName: Option[String],
serde: Option[String],
compressed: Boolean,
properties: Map[String, String]) extends MetadataMapSupport {
properties: Map[String, String],
serdeName: Option[String] = None) extends MetadataMapSupport {

override def toString: String = {
toLinkedHashMap.map { case (key, value) =>
Expand Down Expand Up @@ -181,7 +181,7 @@ case class CatalogStorageFormat(
object CatalogStorageFormat {
/** Empty storage format for default values and copies. */
val empty = CatalogStorageFormat(locationUri = None, inputFormat = None, outputFormat = None,
serdeName = None, serde = None, compressed = false, properties = Map.empty)
serde = None, compressed = false, properties = Map.empty)
}

/**
Expand Down Expand Up @@ -616,11 +616,11 @@ case class CatalogTable(
inputFormat: Option[String] = storage.inputFormat,
outputFormat: Option[String] = storage.outputFormat,
compressed: Boolean = false,
serdeName: Option[String] = storage.serdeName,
serde: Option[String] = storage.serde,
properties: Map[String, String] = storage.properties): CatalogTable = {
properties: Map[String, String] = storage.properties,
serdeName: Option[String] = storage.serdeName): CatalogTable = {
copy(storage = CatalogStorageFormat(
locationUri, inputFormat, outputFormat, serdeName, serde, compressed, properties))
locationUri, inputFormat, outputFormat, serde, compressed, properties, serdeName))
}

def toJsonLinkedHashMap: mutable.LinkedHashMap[String, JValue] = {
Expand Down
1 change: 0 additions & 1 deletion ...lyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogEventSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ class ExternalCatalogEventSuite extends SparkFunSuite {
locationUri = Some(tableUri),
inputFormat = Some("tableInputFormat"),
outputFormat = Some("tableOutputFormat"),
serdeName = None,
serde = None,
compressed = false,
properties = Map.empty)
Expand Down
7 changes: 3 additions & 4 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,7 @@ abstract class ExternalCatalogSuite extends SparkFunSuite {
tableType = CatalogTableType.EXTERNAL,
storage = CatalogStorageFormat(
Some(Utils.createTempDir().toURI),
None, None, None, None, false, Map.empty),
None, None, None, false, Map.empty),
schema = new StructType().add("a", "int").add("b", "string"),
provider = Some(defaultProvider)
)
Expand Down Expand Up @@ -959,7 +959,7 @@ abstract class ExternalCatalogSuite extends SparkFunSuite {
Map("partCol1" -> "7", "partCol2" -> "8"),
CatalogStorageFormat(
Some(tempPath.toURI),
None, None, None, None, false, Map.empty))
None, None, None, false, Map.empty))
catalog.createPartitions("db1", "tbl", Seq(partWithExistingDir), ignoreIfExists = false)

tempPath.delete()
Expand All @@ -968,7 +968,7 @@ abstract class ExternalCatalogSuite extends SparkFunSuite {
Map("partCol1" -> "9", "partCol2" -> "10"),
CatalogStorageFormat(
Some(tempPath.toURI),
None, None, None, None, false, Map.empty))
None, None, None, false, Map.empty))
catalog.createPartitions("db1", "tbl", Seq(partWithNonExistingDir), ignoreIfExists = false)
assert(tempPath.exists())
}
Expand Down Expand Up @@ -1030,7 +1030,6 @@ abstract class CatalogTestUtils {
locationUri = None,
inputFormat = Some(tableInputFormat),
outputFormat = Some(tableOutputFormat),
serdeName = None,
serde = None,
compressed = false,
properties = Map.empty)
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/AlwaysPersistedConfigsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class AlwaysPersistedConfigsSuite extends QueryTest with SharedSparkSession {
val catalogTable = new CatalogTable(
identifier = TableIdentifier(testViewName),
tableType = CatalogTableType.VIEW,
storage = CatalogStorageFormat(None, None, None, None, None, false, Map.empty),
storage = CatalogStorageFormat(None, None, None, None, false, Map.empty),
schema = new StructType(),
properties = Map.empty[String, String]
)
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ trait DDLSuiteBase extends SQLTestUtils {
spec: TablePartitionSpec,
tableName: TableIdentifier): Unit = {
val part = CatalogTablePartition(
spec, CatalogStorageFormat(None, None, None, None, None, false, Map()))
spec, CatalogStorageFormat(None, None, None, None, false, Map()))
catalog.createPartitions(tableName, Seq(part), ignoreIfExists = false)
}

Expand Down
2 changes: 0 additions & 2 deletions sql/core/src/test/scala/org/apache/spark/sql/sources/InsertSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,7 +944,6 @@ class InsertSuite extends DataSourceTest with SharedSparkSession {
locationUri = None,
inputFormat = None,
outputFormat = None,
serdeName = None,
serde = None,
compressed = false,
properties = Map.empty),
Expand Down Expand Up @@ -973,7 +972,6 @@ class InsertSuite extends DataSourceTest with SharedSparkSession {
locationUri = None,
inputFormat = None,
outputFormat = None,
serdeName = None,
serde = None,
compressed = false,
properties = Map.empty),
Expand Down
12 changes: 6 additions & 6 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,23 +177,23 @@ private[hive] class HiveMetastoreCatalog(sparkSession: SparkSession) extends Log
val options = storage.properties + (ParquetOptions.MERGE_SCHEMA ->
SQLConf.get.getConf(HiveUtils.CONVERT_METASTORE_PARQUET_WITH_SCHEMA_MERGING).toString)
storage.copy(
serdeName = None,
Copy link
Contributor

@tagatac tagatac Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if this change was not correct in the original PR, but doesn't unsetting serdeName match the sentiment of unsetting serde here?

Copy link
Contributor Author

@cloud-fan cloud-fan Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just a name for display, so does not really matter.

Copy link
Contributor Author

@cloud-fan cloud-fan Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but still better to clear it.

serde = None,
properties = options
properties = options,
serdeName = None
)
} else {
val options = storage.properties
if (SQLConf.get.getConf(SQLConf.ORC_IMPLEMENTATION) == "native") {
storage.copy(
serdeName = None,
Copy link
Contributor

@tagatac tagatac Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as line 180.

serde = None,
properties = options
properties = options,
serdeName = None
)
} else {
storage.copy(
serdeName = None,
Copy link
Contributor

@tagatac tagatac Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as line 180.

serde = None,
properties = options
properties = options,
serdeName = None
)
}
}
Expand Down
10 changes: 5 additions & 5 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,11 +557,11 @@ private[hive] class HiveClientImpl(
outputFormat = Option(h.getTTable.getSd.getOutputFormat).orElse {
Option(h.getStorageHandler).map(_.getOutputFormatClass.getName)
},
serdeName = Option(h.getTTable.getSd.getSerdeInfo.getName),
serde = Option(h.getSerializationLib),
compressed = h.getTTable.getSd.isCompressed,
properties = Option(h.getTTable.getSd.getSerdeInfo.getParameters)
.map(_.asScala.toMap).orNull
.map(_.asScala.toMap).orNull,
serdeName = Option(h.getTTable.getSd.getSerdeInfo.getName).filter(_.nonEmpty)
),
// For EXTERNAL_TABLE, the table properties has a particular field "EXTERNAL". This is added
// in the function toHiveTable.
Expand Down Expand Up @@ -1202,7 +1202,7 @@ private[hive] object HiveClientImpl extends Logging {
hiveTable.getTTable.getSd.setLocation(loc)}
table.storage.inputFormat.map(toInputFormat).foreach(hiveTable.setInputFormatClass)
table.storage.outputFormat.map(toOutputFormat).foreach(hiveTable.setOutputFormatClass)
table.storage.serdeName.foreach(hiveTable.getSd.getSerdeInfo.setName)
table.storage.serdeName.foreach(hiveTable.getTTable.getSd.getSerdeInfo.setName)
Copy link
Contributor Author

@cloud-fan cloud-fan Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: found all other places use h.getTTable.getSd, so make it consistent here.

hiveTable.setSerializationLib(
table.storage.serde.getOrElse("org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe"))
table.storage.properties.foreach { case (k, v) => hiveTable.setSerdeParam(k, v) }
Expand Down Expand Up @@ -1283,11 +1283,11 @@ private[hive] object HiveClientImpl extends Logging {
locationUri = Option(CatalogUtils.stringToURI(apiPartition.getSd.getLocation)),
inputFormat = Option(apiPartition.getSd.getInputFormat),
outputFormat = Option(apiPartition.getSd.getOutputFormat),
serdeName = Option(apiPartition.getSd.getSerdeInfo.getName),
serde = Option(apiPartition.getSd.getSerdeInfo.getSerializationLib),
compressed = apiPartition.getSd.isCompressed,
properties = Option(apiPartition.getSd.getSerdeInfo.getParameters)
.map(_.asScala.toMap).orNull),
.map(_.asScala.toMap).orNull,
serdeName = Option(apiPartition.getSd.getSerdeInfo.getName).filter(_.nonEmpty)),
createTime = apiPartition.getCreateTime.toLong * 1000,
lastAccessTime = apiPartition.getLastAccessTime.toLong * 1000,
parameters = properties,
Expand Down
12 changes: 12 additions & 0 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveMetastoreCatalogSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,4 +513,16 @@ class DataSourceWithHiveMetastoreCatalogSuite
assert(tableWithSerdeName.storage.serdeName === Some("testSerdeName"))
}
}

test("SPARK-55645: serdeName should be None for tables without an explicit serde name") {
withTable("t") {
sql("CREATE TABLE t (d1 DECIMAL(10,3), d2 STRING) STORED AS TEXTFILE")

val hiveTable =
sessionState.catalog.getTableMetadata(TableIdentifier("t", Some("default")))
// Hive Metastore returns "" for tables without an explicit serde name.
// This should be mapped to None, not Some("").
assert(hiveTable.storage.serdeName === None)
}
}
}
1 change: 0 additions & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSchemaInferenceSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ class HiveSchemaInferenceSuite
locationUri = Option(dir.toURI),
inputFormat = serde.inputFormat,
outputFormat = serde.outputFormat,
serdeName = None,
serde = serde.serde,
compressed = false,
properties = Map("serialization.format" -> "1")),
Expand Down
1 change: 0 additions & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/MetastoreDataSourcesSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,6 @@ class MetastoreDataSourcesSuite extends QueryTest
locationUri = None,
inputFormat = None,
outputFormat = None,
serdeName = None,
serde = None,
compressed = false,
properties = Map(
Expand Down
3 changes: 0 additions & 3 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ class HiveClientSuite(version: String) extends HiveVersionSuite(version) {
locationUri = None,
inputFormat = Some(classOf[TextInputFormat].getName),
outputFormat = Some(classOf[HiveIgnoreKeyTextOutputFormat[_, _]].getName),
serdeName = None,
serde = Some(classOf[LazySimpleSerDe].getName),
compressed = false,
properties = Map.empty
Expand Down Expand Up @@ -370,7 +369,6 @@ class HiveClientSuite(version: String) extends HiveVersionSuite(version) {
locationUri = None,
inputFormat = None,
outputFormat = None,
serdeName = None,
serde = None,
compressed = false,
properties = Map.empty)
Expand All @@ -385,7 +383,6 @@ class HiveClientSuite(version: String) extends HiveVersionSuite(version) {
locationUri = None,
inputFormat = Some(classOf[TextInputFormat].getName),
outputFormat = Some(classOf[HiveIgnoreKeyTextOutputFormat[_, _]].getName),
serdeName = None,
serde = Some(classOf[LazySimpleSerDe].getName),
compressed = false,
properties = Map.empty
Expand Down
1 change: 0 additions & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HivePartitionFilteringSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ class HivePartitionFilteringSuite(version: String)
locationUri = None,
inputFormat = Some(classOf[TextInputFormat].getName),
outputFormat = Some(classOf[HiveIgnoreKeyTextOutputFormat[_, _]].getName),
serdeName = None,
serde = Some(classOf[LazySimpleSerDe].getName()),
compressed = false,
properties = Map.empty
Expand Down
2 changes: 0 additions & 2 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ class HiveDDLSuite
locationUri = Some(catalog.defaultTablePath(name)),
inputFormat = serde.get.inputFormat,
outputFormat = serde.get.outputFormat,
serdeName = None,
serde = serde.get.serde,
compressed = false,
properties = Map.empty)
Expand All @@ -93,7 +92,6 @@ class HiveDDLSuite
locationUri = Some(catalog.defaultTablePath(name)),
inputFormat = Some("org.apache.hadoop.mapred.SequenceFileInputFormat"),
outputFormat = Some("org.apache.hadoop.hive.ql.io.HiveSequenceFileOutputFormat"),
serdeName = None,
serde = Some("org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe"),
compressed = false,
properties = Map("serialization.format" -> "1"))
Expand Down