cloud-fan commented on code in PR #59716:
URL: https://github.com/apache/spark/pull/59716#discussion_r4369808191
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala:
##########
@@ -47,7 +48,8 @@ private case class PostgresDialect()
private val supportedAggregateFunctions = Set("MAX", "MIN", "SUM", "COUNT",
"AVG",
"VAR_POP", "VAR_SAMP", "STDDEV_POP", "STDDEV_SAMP", "COVAR_POP",
"COVAR_SAMP", "CORR",
"REGR_INTERCEPT", "REGR_R2", "REGR_SLOPE", "REGR_SXY")
- private val supportedStringFunctions = Set("RPAD", "LPAD")
+ private val supportedStringFunctions =
+ Set("RPAD", "LPAD", CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING)
Review Comment:
**Blocking (P1):** Opting Postgres in asserts blank-padded comparison for
every column the dialect maps to `CharType`. But `getCatalystType` only diverts
unbounded `bpchar`, so PostgreSQL's internal single-byte `"char"` type also
lands on `CharType(1)` through the `JdbcUtils` default, and that type compares
exactly.
I reproduced this against `postgres:18.2-alpine` at this head:
```sql
CREATE TABLE char_internal (id INT, c "char");
INSERT INTO char_internal VALUES (1, ''), (2, 'a'), (3, ' ');
```
Spark reads `c` as `CHAR(1)` and shows row 1 as `' '`. Then:
- `WHERE c = ' '` is pushed as `(CHAR_READ_SIDE_PADDING(c, 1, ' ')) = ' '`
and returns `{3}`. The unpushed equivalent `concat(c, '') = ' '` returns `{1,
3}`, which is what master returns.
- `WHERE c <> ' '` returns `{1, 2}` pushed versus `{2}` in Spark.
- `WHERE c = 'a'` agrees: `{2}`.
`c = ''` behaves like `c = ' '`, because Spark pads the literal. Spark
removes its own filter for these predicates, so the wrong rows come with no
error. The trigger is narrow: `"char"` is mostly a catalog type, and only its
empty value diverges. `bpchar(n)` columns behave correctly in the same run.
Casting the column to `bpchar` in the pushed SQL doesn't help; in PostgreSQL,
`CAST(c AS bpchar) = ' '` still returns only row 3.
Mapping the internal `"char"` type (type name `char`) to `StringType` in
`PostgresDialect.getCatalystType` would keep it out of both read-side padding
and this compile. The cost is that existing columns change from `CHAR(1)` to
`STRING`, which is worth a migration-guide line. The alternative is to leave
Postgres out of the opt-in.
**Verification:**
- **Regression:** On a Postgres docker table whose internal "char" column
holds '', 'a' and ' ', equality and <> against '' and ' ' return the same rows
as the equivalent predicate evaluated in Spark.
- **Compatibility:** The existing bpchar(n) CHAR pushdown tests still pass
with the same pushed SQL, and the mapping of other Postgres character types is
unchanged.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/OracleDialect.scala:
##########
@@ -46,7 +47,9 @@ 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", CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING)
Review Comment:
**Non-blocking (P2):** With Oracle opted in, `generateCharPadInSet` turns a
large CHAR `IN`/`isin` (one over
`spark.sql.optimizer.inSetConversionThreshold`) into one V2 `IN` carrying every
literal. `OracleSQLBuilder` inherits `V2ExpressionSQLBuilder.visitIn`, which
renders the whole list inline, and nothing in the JDBC path caps it. Oracle
limits an expression list to 1000 entries (ORA-01795), so
`col("code").isin(<1500 codes>)` on a `CHAR(3)` column now fails at the remote.
On master the padded `InSet` had no translation and ran in Spark.
VARCHAR2 and NUMBER columns already hit this limit today. This PR extends it
to CHAR columns, which were safe only because they couldn't be pushed. I didn't
run a >1000-value list against Oracle here; this rests on Oracle's documented
limit.
One option is to override `visitIn` in `OracleSQLBuilder` to emit `(c IN
(<first 1000>) OR c IN (<rest>))` for longer lists. That keeps three-valued
semantics, including under `NOT`, and also fixes the existing non-CHAR case.
**Verification:**
- **Regression:** Compiling a CHAR_READ_SIDE_PADDING IN predicate with more
than 1000 literals for the Oracle dialect yields OR-ed IN chunks of at most
1000 values over the raw column, with every literal present exactly once.
- **Compatibility:** Compiling IN predicates with at most 1000 literals for
the Oracle dialect, including under NOT, produces the same SQL as before the
change.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -417,6 +419,16 @@ abstract class JdbcDialect extends Serializable with
Logging {
override def build(expr: Expression): String = expr match {
case _: AlwaysTrue => "(1 = 1)"
case _: AlwaysFalse => "(1 = 0)"
+ case e: GeneralScalarExpression
+ if e.name() == CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING =>
+ if (!isSupportedFunction(CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING)) {
Review Comment:
**Nit (P3):** Dialects that don't opt in include the base `JdbcDialect`,
MySQL, DB2, Derby, Teradata, Snowflake, Databricks and third-party dialects. On
those, plain CHAR `=`, `<>`, `<=>` and `IN` filters now translate and reach
this throw. `compileExpression` then logs `Error occurs while compiling V2
expression` at WARN with the full stack trace, on every planning. On master
these predicates were never translated, so nothing was logged. Results don't
change, but CHAR flag and code columns are common, and the log reads like a
planning failure.
Declining up front in `JDBCScanBuilder.pushPredicates` would keep this path
quiet: return predicates that contain `CHAR_READ_SIDE_PADDING` as unsupported,
without compiling them, when the dialect doesn't list the name. Opted-in
dialects and the unwrapped `IS [NOT] NULL` stay as they are.
**Verification:**
- **Regression:** Planning a CHAR equality filter against a dialect that
does not opt in leaves the filter post-scan and emits no 'Error occurs while
compiling V2 expression' warning.
- **Compatibility:** Opted-in dialects still push CHAR equality and IN
filters with the same compiled SQL, and every dialect still pushes the
unwrapped IS [NOT] NULL predicate.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/CharVarcharUtils.scala:
##########
@@ -34,6 +34,22 @@ object CharVarcharUtils extends Logging with
SparkCharVarcharUtils {
// visible for testing
private[sql] val CHAR_VARCHAR_TYPE_STRING_METADATA_KEY =
"__CHAR_VARCHAR_TYPE_STRING"
+ /** JVM method name used by [[addPaddingForScan]] when read-side CHAR
padding is on. */
+ private[sql] val READ_SIDE_PADDING = "readSidePadding"
+
+ /** JVM method name used by [[addPaddingForScan]] under standard
CHAR/VARCHAR semantics. */
+ private[sql] val CHAR_TYPE_READ_SIDE_CHECK = "charTypeReadSideCheck"
+
+ private[sql] val VARCHAR_TYPE_READ_SIDE_CHECK = "varcharTypeReadSideCheck"
+
+ /**
+ * V2 function name for CHAR read-side padding. Distinct from SQL `RPAD` so
connectors that
+ * already allowlist `RPAD` for explicit `rpad()` do not inherit CHAR-scan
rewrite. Emitted
+ * only for = / <=> / <> / IN against a string of the CHAR length. JDBC may
compile it as
+ * the column when CHAR storage is already padded.
Review Comment:
**Nit (P3):** This sentence still uses the storage criterion that was
corrected in the `GeneralScalarExpression` entry. MySQL pads `CHAR` on storage
but strips trailing spaces on retrieval and defaults to a NO PAD collation. So
it meets "storage is already padded", yet this PR deliberately keeps it out.
Something like "JDBC may compile it as the column only when the remote compares
CHAR blank-padded (see `JdbcDialect.isSupportedFunction`)" would match the real
condition.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59716#discussion_r4359382468).
See **Shared repair plan 1** in the review body.
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -416,4 +416,27 @@ class PostgresIntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JDBCT
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)")
Review Comment:
**Nit (P3):** These tests, here and in `MsSqlServerIntegrationSuite`, pass
on this head when run, and they do catch an under-matching remote: a NO PAD or
trailing-space-stripping remote would make `= 'Y'` return nothing and `NOT IN`
return id 1. With only `(1, 'Y')` and `(2, NULL)`, though, nothing catches a
predicate that matches too much. `= 'Y'` has no non-null row it must exclude,
and `NOT IN ('Y', 'N')` expects an empty result, which a predicate rejecting
every row also produces. Adding a row like `(3, 'YES')`, and asserting that `=`
excludes it and `NOT IN ('Y', 'N')` returns it, would let both assertions fail
in either direction.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59716#discussion_r4354001519).
See **Shared repair plan 2** in the review body.
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala:
##########
@@ -428,4 +428,36 @@ class OracleIntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JDBCTes
assert(rows11(0).getString(0) === "amy")
assert(rows11(1).getString(0) === "alex")
}
+
+ test("SPARK-60479: CHAR equality is pushed, including CJK") {
+ val tableName = caseConvert("char_pad")
+ val table = s"$catalogName.$tableName"
+ withTable(table) {
+ // CHAR(n CHAR) so CJK occupies one length unit, matching Spark CHAR
padding.
+ Using.resource(getConnection()) { conn =>
+ conn.prepareStatement(
+ s"CREATE TABLE $tableName (id NUMBER, flag CHAR(4
CHAR))").executeUpdate()
Review Comment:
**Nit (P3):** With `CHAR(4 CHAR)`, every stored value is already the same
4-character string Spark pushes, so an exact comparison would pass as well. The
opt-in premise matters for Oracle's default byte-length `CHAR(n BYTE)` with
multibyte data, where the stored value is shorter than Spark's padded literal.
I checked that case against the suite's default image at this head. A `CHAR(4
BYTE)` column holding a two-byte character (U+00E4), alone and followed by `b`,
returned the same rows pushed as through a local `rtrim` predicate. So the
behavior is right; it just isn't covered. A `CHAR(n BYTE)` table with such data
would pin it.
See **Shared repair plan 2** in the review body.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java:
##########
@@ -428,6 +428,21 @@
* <li>Since version: 4.0.0</li>
* </ul>
* </li>
+ * <li>Name: <code>CHAR_READ_SIDE_PADDING</code>
+ * <ul>
+ * <li>SQL semantic: <code>CHAR_READ_SIDE_PADDING(str, len, pad)</code>.
+ * <code>len</code> is an integer literal; <code>pad</code> is always a
+ * single space. Right-pads <code>str</code> with spaces to
+ * <code>len</code> characters; values of <code>len</code> or more
+ * characters are returned unchanged. Spark only emits this in
+ * <code>=</code>, <code><=></code>, <code><></code>, and
+ * <code>IN</code> against a string of length <code>len</code>.
+ * A source may compile it as the raw value only if it compares that
Review Comment:
**Nit (P3):** This permission isn't quite sufficient on its own.
`CHAR_READ_SIDE_PADDING` keeps values of `len` or more characters unchanged, so
for an over-length stored value Spark compares strings of different lengths and
gets false. PAD SPACE comparison instead pads the literal and matches whenever
the value is the literal plus trailing spaces. For example, with a
metadata-only `CHAR(5)` holding `'abcde '`, Spark evaluates `= 'abcde'` as
false, but a source following this text returns the row. JDBC remotes with a
real `CHAR(n)` enforce the length, so only connectors that don't enforce it are
exposed.
Adding one clause, for example "and its stored values never exceed `len`
characters", would make the permission sufficient. That is the stored-length
bound an earlier thread asked to make explicit.
This extends existing discussions
[1](https://github.com/apache/spark/pull/59716#discussion_r4352511490),
[2](https://github.com/apache/spark/pull/59716#discussion_r4359382468).
See **Shared repair plan 1** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2StrategySuite.scala:
##########
@@ -1882,6 +1883,104 @@ class DataSourceV2StrategySuite extends
SharedSparkSession {
}
}
+ test("SPARK-60479: read-side CHAR padding StaticInvoke translates") {
+ val md = new MetadataBuilder()
+ .putString(CharVarcharUtils.CHAR_VARCHAR_TYPE_STRING_METADATA_KEY,
"char(1)")
+ .build()
+ val col = AttributeReference("flag", StringType, metadata = md)()
+ val padded = CharVarcharUtils.addPaddingForScan(col)
+ val eq = EqualTo(padded, Literal("Y"))
+ val result = new V2ExpressionBuilder(eq, isPredicate = true).build()
+ assert(result.isDefined, "readSidePadding should translate to a V2
predicate")
+ result.get match {
+ case p: Predicate if p.name() == "=" =>
+ p.children()(0) match {
+ case g: GeneralScalarExpression
+ if g.name() == CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING =>
+ assert(g.children()(0) === FieldReference("flag"))
+ assert(g.children()(1) === LiteralValue(1, IntegerType))
+ assert(g.children()(2) ===
+ LiteralValue(UTF8String.fromString(" "), StringType))
+ case other =>
+ fail(s"expected CHAR_READ_SIDE_PADDING, got ${other.describe()}")
+ }
+ assert(p.children()(1) === LiteralValue(UTF8String.fromString("Y"),
StringType))
+ case other =>
+ fail(s"expected '=' predicate, got ${other.describe()}")
+ }
+
+ val isNull = new V2ExpressionBuilder(IsNull(padded), isPredicate =
true).build()
+ assert(isNull.isDefined)
+ isNull.get match {
+ case p: Predicate if p.name() == "IS_NULL" =>
+ assert(p.children()(0) === FieldReference("flag"))
+ case other => fail(s"expected IS_NULL on the raw column, got
${other.describe()}")
+ }
+
+ val nseq = new V2ExpressionBuilder(EqualNullSafe(padded, Literal("Y")),
isPredicate = true)
+ .build()
+ assert(nseq.isDefined)
+ val in = new V2ExpressionBuilder(In(padded, Seq(Literal("Y"))),
isPredicate = true).build()
+ assert(in.isDefined)
+ val neq = new V2ExpressionBuilder(Not(EqualTo(padded, Literal("Y"))),
isPredicate = true)
+ .build()
+ assert(neq.isDefined)
+ neq.get match {
+ case p: Predicate if p.name() == "<>" =>
Review Comment:
**Nit (P3):** This case only checks the predicate name. The `=` and `InSet`
cases above assert the `CHAR_READ_SIDE_PADDING` child and the padded literal,
and that wrapper on child 0 is what keeps non-opted dialects from pushing a
raw-column comparison. The `<>` arm shares `generateCharPadLiteralCmp` with
`=`, so the gap is narrow, but a change to that arm that dropped the wrapper or
the padding would still pass. No H2 or docker query runs `flag <> 'Y'` end to
end either. Mirroring the `=` assertions here, and adding an H2 `flag <> 'Y'`
query with `checkPushedInfo` and `checkAnswer` (the NULL row must not come
back), would pin it.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59716#discussion_r4354620406).
**Verification:**
- **Regression:** The <> translation is asserted structurally, wrapper child
and padded literal, so emitting a raw column reference or an unpadded literal
fails.
- **Behavior:** An end-to-end CHAR <> query pushes the wrapped predicate,
leaves no post-scan filter, and returns only the non-null rows that differ from
the literal.
--
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]
