spark-test commented on code in PR #59716:
URL: https://github.com/apache/spark/pull/59716#discussion_r4359382468
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java:
##########
@@ -428,6 +428,17 @@
* <li>Since version: 4.0.0</li>
* </ul>
* </li>
+ * <li>Name: <code>CHAR_READ_SIDE_PADDING</code>
+ * <ul>
+ * <li>SQL semantic: right-pads <code>str</code> with spaces to
<code>len</code>
Review Comment:
**Nit (P3):** Unlike its neighbors (`RPAD(str, len[, pad])`, for example),
this entry gives no invocation form. Spark always emits three children: the
column, an integer length literal, and a `' '` pad. JDBCV2Suite pins that as
`CHAR_READ_SIDE_PADDING(flag, 5, ' ')`. A connector author working from this
list can't tell the arity or child order.
The last sentence also ties the raw-column compile to storage ('store CHAR
already padded'), but what makes it equivalent is blank-padded comparison.
MySQL stores CHAR padded, yet it strips trailing spaces on retrieval and its
default collation is NO PAD. That's why this PR keeps MySQL out. A connector
following this text on such a backend would drop rows from pushed CHAR equality
and `IN` filters.
Something like `CHAR_READ_SIDE_PADDING(str, len, pad)` (`len` an integer
literal, `pad` always a single space) would fix the first part. The second
could say a source may compile it as the raw value only if it compares that
value with a `len`-character string using blank-padded (PAD SPACE) semantics.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59716#pullrequestreview-5621417839).
**Verification:**
- **Inspection:** The documented invocation form, child order, and child
roles match the CHAR read-side padding expression Spark emits: the column, an
integer length literal, and a single-space pad.
- **Inspection:** The documented raw-value condition is stated in terms of
blank-padded comparison, and a backend that strips trailing spaces and compares
with a NO PAD collation does not satisfy it.
##########
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()
+ }
+ // scalastyle:off nonascii
+ sql(s"INSERT INTO $table VALUES (1, 'Y'), (2, NULL), (3, '日本')")
+ // scalastyle:on nonascii
+ val eq = sql(s"SELECT id FROM $table WHERE flag = 'Y'")
+ checkFilterPushed(eq)
+ val eqRows = eq.collect()
+ assert(eqRows.length === 1)
+ assert(eqRows(0).getInt(0) === 1)
Review Comment:
**Non-blocking (P2):** `id NUMBER` without precision reads back as
`DecimalType(38, 18)`: `OracleDialect.getCatalystType` maps JDBC precision 0 to
that unless `spark.sql.legacy.oracle.numberMapping.enabled` is set.
`Row.getInt` is `get(i).asInstanceOf[Int]`, so unboxing the
`java.math.BigDecimal` throws `ClassCastException` right after the first
`checkFilterPushed`. The same applies at the CJK and `IS NULL` checks below.
The `caseConvert` fix addresses the earlier `TABLE_OR_VIEW_NOT_FOUND`, but
the next Oracle Docker run will fail here. The equality, CJK, and `IS NULL`
paths still won't have been checked against Oracle, and passing this test was
the approval's merge condition. Declaring `NUMBER(10)` or `INTEGER` still maps
to a decimal, so reading the value as a decimal, e.g.
`getDecimal(0).intValueExact()`, is the simpler fix.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59716#pullrequestreview-5622188304).
**Verification:**
- **Behavior:** Oracle Docker integration coverage for CHAR read-side
padding shows the equality, CJK equality, and IS NULL queries pushed to Oracle
and returning ids 1, 3, and 2 respectively.
- **Regression:** The id values are read consistently with their decimal
type, so the Oracle CHAR test fails only when pushdown or the returned rows are
wrong, never because of the id column's Spark type.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2StrategySuite.scala:
##########
@@ -1882,6 +1883,87 @@ 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() == "<>" =>
+ case other => fail(s"expected '<>' predicate, got ${other.describe()}")
+ }
+ val inSet = new V2ExpressionBuilder(
+ InSet(padded, Set(UTF8String.fromString("Y"))), isPredicate =
true).build()
+ assert(inSet.isDefined)
Review Comment:
**Nit (P3):** This only checks that some predicate comes back.
`generateCharPadInSet` builds its own operands instead of going through the
`In` path. If it emitted the raw column, or literals of the wrong value or
type, this would still pass. Nothing else reaches the branch: every IN list in
JDBCV2Suite and the Docker suites has at most two literals, below the default
`spark.sql.optimizer.inSetConversionThreshold` of 10, so `OptimizeIn` leaves
them as `In`. Asserting the full shape here, and adding an H2 CHAR `IN` query
with more than 10 literals (or a lowered threshold) to JDBCV2Suite, would cover
what CHAR `isin()` over many codes actually runs.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59716#discussion_r4354620406).
**Verification:**
- **Regression:** If the InSet translation emitted the raw column or
unpadded literals instead of CHAR_READ_SIDE_PADDING over padded literals, the
translation coverage fails.
- **Behavior:** An H2 CHAR query whose IN list is converted to InSet shows
CHAR_READ_SIDE_PADDING IN in its pushed filters, has no remaining post-scan
filter, and returns the expected rows.
--
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]
