dtenedor commented on code in PR #59716:
URL: https://github.com/apache/spark/pull/59716#discussion_r4354620388
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala:
##########
@@ -428,4 +428,35 @@ 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 table = s"$catalogName.char_pad"
Review Comment:
This is why the Oracle Docker job [failed on
`3d24a8c`](https://github.com/srielau/spark/actions/runs/39847088203/job/120074532366):
`TABLE_OR_VIEW_NOT_FOUND` for `oracle`.`char_pad` at the `INSERT`. The
unquoted `CREATE TABLE char_pad` over JDBC creates `CHAR_PAD`, and this suite
overrides `caseConvert` to upper-case names for exactly that reason.
`s"$catalogName.${caseConvert("char_pad")}"` should fix it. Please make sure
this passes before merge, since it's the only test that exercises the Oracle
path, including the CJK case.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/GeneralScalarExpression.java:
##########
@@ -428,6 +428,16 @@
* <li>Since version: 4.0.0</li>
* </ul>
* </li>
+ * <li>Name: <code>CHAR_READ_SIDE_PADDING</code>
+ * <ul>
+ * <li>SQL semantic: <code>RPAD(str, len, ' ')</code> on a CHAR scan column
when
+ * read-side padding is on. Spark only emits this in <code>=</code>,
+ * <code><=></code>, <code><></code>, and <code>IN</code>
against
+ * a string of length <code>len</code>. JDBC sources that store CHAR
already
+ * padded may compile it as the raw column.</li>
Review Comment:
`RPAD` truncates values longer than `len`, but read-side padding returns
them unchanged (`CharVarcharCodegenUtils.readSidePadding` is pad-or-keep).
Since connector authors implement against this text, could it say something
like "right-pads `str` with spaces to `len` characters; values of `len` or more
characters are returned unchanged"?
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2StrategySuite.scala:
##########
@@ -1882,6 +1883,71 @@ 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)
Review Comment:
nit: it would be worth also covering `Not(EqualTo(...))` (`<>`), `InSet`, an
`IN` list containing a NULL literal (it should stay local without throwing),
and the literal on the left side (`EqualTo(Literal("Y"), padded)`).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -458,6 +478,91 @@ class V2ExpressionBuilder(e: Expression, isPredicate:
Boolean = false) extends L
}
}
+ private def isCharReadSidePaddingInvoke(invoke: StaticInvoke): Boolean = {
+ invoke.scalarFunction.isEmpty &&
+ invoke.staticObject == classOf[CharVarcharCodegenUtils] &&
+ invoke.functionName == CharVarcharUtils.READ_SIDE_PADDING &&
+ invoke.arguments.length == 2
+ }
+
+ // Identity compile is only valid vs a literal Spark already padded to the
CHAR length.
+ private def charPadLength(invoke: StaticInvoke): Option[Int] = {
+ val len = invoke.arguments(1)
+ if (len.foldable) {
+ len.eval() match {
+ case n: Int => Some(n)
+ case _ => None
+ }
+ } else {
+ None
+ }
+ }
+
+ private def isFoldableStringOfLen(expr: Expression, n: Int): Boolean = {
+ expr.foldable && (expr.eval() match {
Review Comment:
nit: this calls `eval()` on any foldable expression. After constant folding
the padded literal is already a `Literal`, but `ConstantFolding` deliberately
leaves an expression unfolded when it fails inside a conditional branch, and
evaluating one here would throw during planning. Matching `Literal(s:
UTF8String, _)` instead would avoid that.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -467,7 +479,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 CharVarcharUtils.V2_CHAR_READ_SIDE_PADDING => inputs(0)
Review Comment:
nit: this branch looks unreachable now. `build` handles
`CHAR_READ_SIDE_PADDING` before it gets here, and `V2ExpressionSQLBuilder` no
longer routes the name to `visitSQLFunction`. I'd drop it and its comment.
--
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]
