srielau commented on code in PR #58255:
URL: https://github.com/apache/spark/pull/58255#discussion_r3855006059
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveShim.scala:
##########
@@ -408,7 +408,14 @@ private[client] class Shim_v2_0 extends Shim with Logging {
}
}
- if (!SQLConf.get.metastorePartitionPruningFastFallback ||
+ // CHAR/VARCHAR partition keys are excluded from the metastore filter (see
+ // SupportedAttribute), because Hive compares them with its own
trailing-blank rules. Under
+ // standard semantics that would leave such a query fetching every
partition, so prune on the
+ // client instead, where Spark's own comparison semantics apply.
+ val charVarcharPartitionKey = SQLConf.get.charVarcharStandardSemantics &&
+ catalogTable.partitionSchema.exists(f =>
CharVarcharUtils.hasCharVarchar(f.dataType))
+
+ if ((!SQLConf.get.metastorePartitionPruningFastFallback &&
!charVarcharPartitionKey) ||
Review Comment:
This forces client-side prune whenever the table has a CHAR/VARCHAR
partition key, not only when the predicate references one. INT-only filters
still go through `convertFilters`, but other empty-filter / MetaException
fallbacks on such tables ignore `metastorePartitionPruningFastFallback=false`
(the default).
Could we key this off predicates that actually mention a CHAR/VARCHAR
partition attribute, and leave the MetaException path on the existing conf? A
positive condition would also be easier to read than `(!fastFallback &&
!charKey)`.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2219,6 +2256,26 @@ class FileSourceCharVarcharTestSuite extends
CharVarcharTestSuite with SharedSpa
checkAnswer(sql("SELECT * FROM t"), Row("12"))
}
}
+ // Catalog write/read and file inference both keep CHAR/VARCHAR.
+ withTable("std_parquet") {
+ sql(s"CREATE TABLE std_parquet (c CHAR(5), v VARCHAR(5)) USING
$format")
+ sql("INSERT INTO std_parquet VALUES ('ab', 'cd')")
+ assert(spark.table("std_parquet").schema.map(_.dataType) ===
+ Seq(CharType(5), VarcharType(5)))
+ checkAnswer(
+ sql("SELECT concat('<', c, '>'), concat('<', v, '>') FROM
std_parquet"),
+ Row("<ab >", "<cd>"))
+ }
+ withTempPath { dir =>
+ val path = dir.getCanonicalPath
+ sql("SELECT CAST('ab' AS CHAR(4)) AS
c").write.mode("overwrite").format(format).save(path)
+ val inferred = spark.read.format(format).load(path)
+ assert(inferred.schema.head.dataType === CharType(4))
+ checkAnswer(inferred.selectExpr("concat('<', c, '>')"), Row("<ab >"))
+ val catalog = spark.read.schema("c
VARCHAR(4)").format(format).load(path)
+ assert(catalog.schema.head.dataType === VarcharType(4))
+ checkAnswer(catalog, Row("ab "))
+ }
Review Comment:
These catalog/inference checks do not hit Empty2Null, TextTable, or Hive
prune, and they sit inside the SPARK-58801 scan-pad test. Could they move to
the PR that preserved Parquet CHAR/VARCHAR, as a separate test?
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveCharVarcharTestSuite.scala:
##########
@@ -91,6 +93,41 @@ class HiveCharVarcharTestSuite extends CharVarcharTestSuite
with TestHiveSinglet
}
}
}
+
+ test("SPARK-58794: CHAR partition filters use the same Hive prune path as
STRING") {
+ // Keep the relation a HiveTableRelation, otherwise the scan is converted
to a file index
+ // and never reaches HiveShim's metastore filter conversion.
+ withSQLConf(
+ SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true",
+ SQLConf.HIVE_METASTORE_PARTITION_PRUNING.key -> "true",
+ HiveUtils.CONVERT_METASTORE_PARQUET.key -> "false") {
+ val partitionValues = Seq("a", "b", "c", "d", "e")
+
+ def partitionsFetched(partitionType: String, literal: String): Long = {
+ var fetched = 0L
+ withTable("std_hive_part") {
+ sql(
+ s"""CREATE TABLE std_hive_part (i INT, p $partitionType)
+ |USING $format PARTITIONED BY (p)""".stripMargin)
+ partitionValues.foreach { v =>
+ sql(s"INSERT INTO std_hive_part PARTITION (p='$v') VALUES (1)")
+ }
+ HiveCatalogMetrics.reset()
+ checkAnswer(sql(s"SELECT i FROM std_hive_part WHERE p = $literal"),
Row(1))
+ fetched = HiveCatalogMetrics.METRIC_PARTITIONS_FETCHED.getCount
+ }
+ fetched
+ }
+
+ val stringFetched = partitionsFetched("STRING", "'a'")
+ // Standard semantics compare CHAR without PAD SPACE, so the literal
carries the pad.
+ val charFetched = partitionsFetched("CHAR(5)", "'a '")
+ assert(stringFetched < partitionValues.length,
+ s"STRING baseline did not prune: fetched $stringFetched of
${partitionValues.length}")
+ assert(charFetched === stringFetched,
+ s"CHAR fetched $charFetched partitions but STRING fetched
$stringFetched")
Review Comment:
CHAR is not on the same Hive metastore filter path as STRING
(`SupportedAttribute` still rejects it; we prune client-side). Could we rename
the test, assert the fetched count is 1, cover VARCHAR, and add `WHERE p = 'a'`
(no match under standard semantics) vs the padded literal?
Also, this PR is SPARK-59001; the test name still says SPARK-58794 (same for
the commit headline and the other new tests).
--
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]
