srielau commented on code in PR #59716:
URL: https://github.com/apache/spark/pull/59716#discussion_r4354498360
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -377,6 +383,16 @@ class V2ExpressionBuilder(e: Expression, isPredicate:
Boolean = false) extends L
case _ =>
None
}
+ // CHAR scan padding is a JVM invoke. Emit CHAR_READ_SIDE_PADDING, not SQL
RPAD: JDBC
+ // compiles it as the column (CHAR is already padded).
charTypeReadSideCheck stays
+ // local so length errors are not dropped.
+ case invoke @ StaticInvoke(_, dataType, _, arguments, _, _, _, _, None)
+ if isCharReadSidePaddingInvoke(invoke) =>
Review Comment:
Thanks. The later commits do this: CHAR_READ_SIDE_PADDING is translated only
as an operand of = / <=> / <> / IN / InSet against foldable strings whose
numChars is exactly n, plus the IS [NOT] NULL unwrap. A bare padding invoke is
not compiled. LIKE 'Y' after LikeSimplification is pad(flag) = 'Y' with a
1-char literal, so it stays local; same for EndsWith, CHAR vs VARCHAR, and RPAD
wrapping a longer literal.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -467,7 +467,12 @@ abstract class JdbcDialect extends Serializable with
Logging {
}
override def visitSQLFunction(funcName: String, inputs: Array[String]):
String = {
- s"""${dialectFunctionName(funcName)}(${inputs.mkString(", ")})"""
+ funcName match {
+ // JDBC CHAR is stored padded. readSidePadding wrapped the column and
blocked
+ // pushdown; compiling it as the column restores col = 'Y '
(literal padded).
+ case "CHAR_READ_SIDE_PADDING" => inputs(0)
Review Comment:
Done. JDBCSQLBuilder and the opt-in dialects use
CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -538,7 +543,8 @@ abstract class JdbcDialect extends Serializable with
Logging {
* @return True if the database supports function.
*/
@Since("3.3.0")
- def isSupportedFunction(funcName: String): Boolean = false
+ def isSupportedFunction(funcName: String): Boolean =
+ funcName == "CHAR_READ_SIDE_PADDING"
Review Comment:
Default is false again. Only H2, Postgres, SQL Server, and Oracle list the
function. Noop/Aggregated and unrecognized URLs do not compile it. We did not
restore the kill switch: the earlier default-on identity was the regression,
and this is now opt-in plus the comparison-shape gate.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:
##########
@@ -45,7 +45,8 @@ private case class MySQLDialect() extends JdbcDialect with
SQLConfHelper with No
// See https://dev.mysql.com/doc/refman/8.0/en/aggregate-functions.html
private val supportedAggregateFunctions =
Set("MAX", "MIN", "SUM", "COUNT", "AVG") ++
distinctUnsupportedAggregateFunctions
- private val supportedFunctions = supportedAggregateFunctions ++
Set("DATE_ADD", "DATE_DIFF")
+ private val supportedFunctions =
+ supportedAggregateFunctions ++ Set("DATE_ADD", "DATE_DIFF",
"CHAR_READ_SIDE_PADDING")
Review Comment:
MySQL is out. compileExpression is empty for the mysql URL, and MySQLDialect
does not list the function.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -153,7 +153,8 @@ yield visitBinaryArithmetic(
"COT", "ASIN", "ASINH", "ACOS", "ACOSH", "ATAN", "ATANH", "ATAN2",
"CBRT", "DEGREES",
"RADIANS", "SIGN", "WIDTH_BUCKET", "SUBSTRING", "UPPER", "LOWER",
"TRANSLATE",
"DATE_ADD", "DATE_DIFF", "TRUNC", "AES_ENCRYPT", "AES_DECRYPT",
"SHA1", "SHA2", "MD5",
- "CRC32", "BIT_LENGTH", "CHAR_LENGTH", "CONCAT", "RPAD", "LPAD" ->
+ "CRC32", "BIT_LENGTH", "CHAR_LENGTH", "CONCAT", "RPAD", "LPAD",
+ "CHAR_READ_SIDE_PADDING" ->
Review Comment:
Documented on GeneralScalarExpression (since 4.4.0). V2ExpressionSQLBuilder
does not allowlist CHAR_READ_SIDE_PADDING, so a generic builder fails instead
of emitting a backend function call.
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala:
##########
@@ -2017,6 +2017,87 @@ class JDBCV2Suite extends SharedSparkSession with
ExplainSuiteHelper {
checkAnswer(df10, Seq(Row(6, "jen", 12000, 1200, true)))
}
+ test("SPARK-60479: CHAR equality is pushed down with read-side padding") {
+ val table = "h2.test.char_pad"
+ withTable(table) {
+ sql(s"CREATE TABLE $table (id INT, flag CHAR(5))")
+ // scalastyle:off nonascii
+ sql(s"INSERT INTO $table VALUES (1, 'Y'), (2, 'N'), (3, 'YES'), (4,
NULL), (5, 'รค')")
+
+ val eq = sql(s"SELECT id FROM $table WHERE flag = 'Y'")
+ checkFiltersRemoved(eq)
+ checkPushedInfo(eq,
+ "PushedFilters: [flag IS NOT NULL, (CHAR_READ_SIDE_PADDING(flag, 5, '
')) = 'Y ']")
+ checkAnswer(eq, Seq(Row(1)))
+
+ val in = sql(s"SELECT id FROM $table WHERE flag IN ('Y', 'N')")
+ checkFiltersRemoved(in)
+ checkPushedInfo(in,
+ "PushedFilters: [CHAR_READ_SIDE_PADDING(flag, 5, ' ') IN ('Y ', 'N
')]")
+ checkAnswer(in, Seq(Row(1), Row(2)))
+
+ val isNull = sql(s"SELECT id FROM $table WHERE flag IS NULL")
+ checkFiltersRemoved(isNull)
+ checkPushedInfo(isNull, "PushedFilters: [flag IS NULL]")
+ checkAnswer(isNull, Seq(Row(4)))
+
+ val notIn = sql(s"SELECT id FROM $table WHERE flag NOT IN ('Y', 'N')")
+ checkFiltersRemoved(notIn)
+ checkPushedInfo(notIn,
+ "PushedFilters: [NOT (CHAR_READ_SIDE_PADDING(flag, 5, ' ') IN ('Y
', 'N '))]")
+ checkAnswer(notIn, Seq(Row(3), Row(5)))
+
+ val orPred = sql(s"SELECT id FROM $table WHERE flag = 'Y' OR flag = 'N'")
+ checkFiltersRemoved(orPred)
+ checkAnswer(orPred, Seq(Row(1), Row(2)))
+
+ val nseq = sql(s"SELECT id FROM $table WHERE flag <=> 'Y'")
+ checkFiltersRemoved(nseq)
+ checkAnswer(nseq, Seq(Row(1)))
+
+ val longLit = sql(s"SELECT id FROM $table WHERE flag = 'YYYYYY'")
+ checkFiltersRemoved(longLit)
+ checkAnswer(longLit, Seq.empty)
+
+ val range = sql(s"SELECT id FROM $table WHERE flag > 'A'")
+ checkFiltersRemoved(range)
+ checkAnswer(range, Seq(Row(1), Row(2), Row(3), Row(5)))
+
+ val likeFam = sql(s"SELECT id FROM $table WHERE flag LIKE 'Y%'")
Review Comment:
Added LIKE 'Y' and LIKE '%Y'; both stay local and return no rows. CHAR vs
VARCHAR is covered by flag = v on a CHAR column vs a VARCHAR column.
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -294,4 +294,25 @@ class MsSqlServerIntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JD
assert(rows.length == 1)
assert(rows(0).getString(0) === "amy")
}
+
+ test("SPARK-60479: CHAR read-side padding with NULL") {
+ val table = s"$catalogName.char_pad"
+ withTable(table) {
+ sql(s"CREATE TABLE $table (id INT, flag CHAR(5))")
+ sql(s"INSERT INTO $table VALUES (1, 'Y'), (2, NULL)")
+ val isNull = sql(s"SELECT id FROM $table WHERE flag IS NULL")
+ checkFilterPushed(isNull)
+ val nullRows = isNull.collect()
+ assert(nullRows.length === 1)
+ assert(nullRows(0).getInt(0) === 2)
+ val notIn = sql(s"SELECT id FROM $table WHERE flag NOT IN ('Y')")
Review Comment:
Both docker suites now use NOT IN ('Y', 'N') so OptimizeIn cannot rewrite it
to a single equality.
--
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]
