dtenedor commented on code in PR #59716:
URL: https://github.com/apache/spark/pull/59716#discussion_r4354001856
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -377,6 +377,17 @@ class V2ExpressionBuilder(e: Expression, isPredicate:
Boolean = false) extends L
case _ =>
None
}
+ // Scan CHAR padding is a JVM invoke; V2 only has RPAD. This matches local
eval when the
+ // stored length is <= n (pad-or-keep then equals truncating RPAD).
Over-long values would
+ // be truncated remotely. charTypeReadSideCheck is left untranslated so
length errors stay
Review Comment:
Compiling to the column resolves the over-long case for JDBC, since the
remote compares the raw value. I'd still like the kill switch back, given how
many dialects this now reaches (see my comment on `isSupportedFunction`).
##########
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:
Compiling this to the bare column is only equivalent to the padded value
when the padding is a direct operand of a comparison or `IN` whose literals
`ApplyCharTypePadding` already padded to `n` characters. In any other position
the padding just disappears, and these return different rows once pushed:
- `flag LIKE 'Y'` matches nothing in Spark, since `'Y '` isn't `'Y'`. But
`LikeSimplification` rewrites it to `readSidePadding(flag, 5) = 'Y'` after
analysis, so the literal is never padded, and it's pushed as `"flag" = 'Y'`.
Oracle, SQL Server, Postgres, DB2, and MySQL all treat that as a match.
- `flag LIKE '%Y'` becomes `ENDS_WITH`. SQL Server [ignores trailing blanks
in the matched
value](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/like-transact-sql),
and MySQL strips them on retrieval, so `'Y '` matches.
- For `char_col = varchar_col`, Spark pads neither side
(`typeWithWiderCharLength` returns `NullType`), so `'Y ' = 'Y'` is false.
Pushed, it's true on SQL Server, MySQL, and Postgres.
- A literal longer than `n` wraps the column in `rpad`. `flag NOT IN ('Y',
'UNKNOWN')` on `CHAR(5)` becomes `NOT (RPAD(CHAR_READ_SIDE_PADDING(flag, 5, '
'), 7, ' ') IN (...))`, which SQL Server compiles to `LEFT(CONCAT("flag",
REPLICATE(' ', 7)), 7)`, so the NULL row comes back.
Could the translation fire only in that comparison shape, plus the `IS [NOT]
NULL` unwrap, and return `None` everywhere else? For example, handle
`BinaryComparison`, `In`, and `InSet` explicitly when every literal has
`numChars == n`, and let a bare padding invoke fall through. That covers all
four cases, and it means `CHAR_READ_SIDE_PADDING` only reaches connectors in
the one shape where compiling it to the column is valid.
##########
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:
I'd leave MySQL out. MySQL's
[docs](https://dev.mysql.com/doc/refman/8.0/en/char.html) say CHAR values are
"retrieved, trailing spaces are removed", and that "NO PAD collations treat
trailing spaces as significant in comparisons". MySQL 8's default `utf8mb4`
collation, `utf8mb4_0900_ai_ci`, is NO PAD. So `flag = 'Y '` doesn't match a
stored `'Y'`, and equality, `IN`, and range filters on CHAR columns would lose
rows that are returned correctly today. Pushed `GROUP BY`/`MIN`/`MAX` would
also return MySQL's stripped values instead of Spark's padded ones. If MySQL
stays in, it needs a `MySQLIntegrationSuite` test that uses the default
collation.
##########
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:
This changes the default of a public `@Since("3.3.0")` method, so every
dialect that doesn't override it now pushes CHAR padding as the bare column.
That includes `NoopDialect`, which handles every unrecognized JDBC URL, plus
`AggregatedDialect`, `SnowflakeDialect`, `DatabricksDialect`, and any
third-party dialect. Spark doesn't know whether those databases compare CHAR
with blank padding (MySQL, for one, doesn't by default). Could the default stay
`false`, with only the built-ins whose CHAR semantics are verified opting in?
I'd also bring back the internal kill switch from `fc4b86d`, since this changes
the pushed SQL for existing queries across dialects.
##########
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:
`CHAR_READ_SIDE_PADDING` is now part of the public V2 expression vocabulary,
but it isn't in the `GeneralScalarExpression` Javadoc, which is the list
connector authors work from. Could you document it there, with its arguments,
semantics (pad-or-keep to `n` with spaces), and a since version? Also,
rendering it here as an ordinary function call means a connector that builds
SQL with `V2ExpressionSQLBuilder` and no allowlist would send
`CHAR_READ_SIDE_PADDING(...)` to a backend that has no such function.
##########
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:
Could you add the cases where the bare column diverges from the padded
value? Those are `flag LIKE 'Y'` and `flag LIKE '%Y'` (both should return no
rows) and a comparison against a VARCHAR column. With the restriction from my
comment on the `V2ExpressionBuilder` case, they should stay local, and
asserting that would pin it.
##########
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:
With a single element, `OptimizeIn` turns this into `NOT (flag = 'Y ')`,
which picks up the inferred `flag IS NOT NULL` guard. So it doesn't exercise
the real `NOT IN` path, which has no guard. `NOT IN ('Y', 'N')` would. The
Postgres suite has the same issue.
##########
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:
nit: `CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING` exists, but the name is
repeated as a string literal here, in the default below, and in eight dialect
files. Using the constant would keep them in sync.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -377,6 +377,17 @@ class V2ExpressionBuilder(e: Expression, isPredicate:
Boolean = false) extends L
case _ =>
None
}
+ // Scan CHAR padding is a JVM invoke; V2 only has RPAD. This matches local
eval when the
+ // stored length is <= n (pad-or-keep then equals truncating RPAD).
Over-long values would
+ // be truncated remotely. charTypeReadSideCheck is left untranslated so
length errors stay
+ // local under spark.sql.charVarchar.standardSemantics.enabled.
+ case StaticInvoke(staticObject, dataType, "readSidePadding", arguments, _,
_, _, _, None)
+ if staticObject == classOf[CharVarcharCodegenUtils] &&
arguments.length == 2 =>
Review Comment:
The opt-in in `fc4b86d` was what I was after. The follow-up makes this the
default for every dialect, including `NoopDialect` through the new
`isSupportedFunction` default, and pushes the padding in positions where the
bare column isn't equivalent. Details are in my new comments on
`JdbcDialects.scala` and the `V2ExpressionBuilder` case.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -377,6 +377,17 @@ class V2ExpressionBuilder(e: Expression, isPredicate:
Boolean = false) extends L
case _ =>
None
}
+ // Scan CHAR padding is a JVM invoke; V2 only has RPAD. This matches local
eval when the
+ // stored length is <= n (pad-or-keep then equals truncating RPAD).
Over-long values would
+ // be truncated remotely. charTypeReadSideCheck is left untranslated so
length errors stay
+ // local under spark.sql.charVarchar.standardSemantics.enabled.
+ case StaticInvoke(staticObject, dataType, "readSidePadding", arguments, _,
_, _, _, None)
+ if staticObject == classOf[CharVarcharCodegenUtils] &&
arguments.length == 2 =>
+ generateExpressionWithNameByChildren(
+ "RPAD",
Review Comment:
`flag = 'Y '` and `flag IS NULL` no longer go through `CONCAT`, but a
literal longer than the column still does. `flag NOT IN ('Y', 'UNKNOWN')` on
`CHAR(5)` pads the column side with `rpad(..., 7)`. That compiles to
`LEFT(CONCAT("flag", REPLICATE(' ', 7)), 7)` and returns the NULL row.
Restricting the translation to direct comparisons, as in my comment on the new
`V2ExpressionBuilder` case, would stop that. So would keeping the NULL-safe
emulation.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/OracleDialect.scala:
##########
@@ -46,7 +46,7 @@ private case class OracleDialect() extends JdbcDialect with
SQLConfHelper with N
// scalastyle:on line.size.limit
private val supportedAggregateFunctions =
Set("MAX", "MIN", "SUM", "COUNT", "AVG") ++
distinctUnsupportedAggregateFunctions
- private val supportedFunctions = supportedAggregateFunctions ++ Set("TRUNC")
+ private val supportedFunctions = supportedAggregateFunctions ++ Set("TRUNC",
"RPAD")
Review Comment:
Agreed that compiling to the column is right for Oracle. CHAR-vs-literal
comparison is blank-padded, so the CJK case works without `RPAD`, and the new
Docker test covers it. What's left for Oracle is the position issue: `flag LIKE
'Y'` is pushed as `"flag" = 'Y'`, which Oracle treats as a match (see my
comment on the `V2ExpressionBuilder` 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: [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]
