srielau commented on code in PR #59716:
URL: https://github.com/apache/spark/pull/59716#discussion_r4352708205
##########
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:
Done: CHAR scan padding is no longer rewritten as V2 `RPAD`. It is
`CHAR_READ_SIDE_PADDING`, and only H2, Postgres, and SQL Server opt in. H2
tests now cover `COUNT`, `GROUP BY`, and top-N as well as filters. The internal
SQLConf above can turn the translation off without disabling read-side padding.
##########
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:
This was the blocking case. SQL Server
`RPAD`/`LPAD`/`CHAR_READ_SIDE_PADDING` now compile as `CASE WHEN str IS NULL OR
len IS NULL OR pad IS NULL THEN NULL ELSE LEFT/RIGHT(CONCAT(...), len) END`.
`IsNull`/`IsNotNull` over `readSidePadding` translate to the raw column, so
`flag IS NULL` is `flag IS NULL` rather than `RPAD(flag) IS NULL`. Added H2
coverage plus a `MsSqlServerIntegrationSuite` CHAR+NULL case (`IS NULL`, `NOT
IN`, `COUNT`, `GROUP BY`). I have not run the docker suite here.
##########
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:
Agreed that RPAD is not an exact match for pad-or-keep. This now emits a
dedicated V2 name `CHAR_READ_SIDE_PADDING` instead of SQL `RPAD`, so a
connector that only allowlists `RPAD` does not pick up CHAR-scan rewrite.
Dialects opt in via `isSupportedFunction`; the CASE/CHAR_LENGTH form would
still be an approximation for over-long values and needs more function
coverage. I also added `spark.sql.optimizer.pushDownCharReadSidePadding` as a
kill switch.
##########
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",
+ Seq(arguments.head, arguments(1), Literal.create(" ",
arguments.head.dataType)),
Review Comment:
The pad literal is now `Literal.create(" ", StringType)`, and the JVM method
name is `CharVarcharUtils.READ_SIDE_PADDING` shared with `addPaddingForScan`.
##########
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:
Oracle is out of this PR: `RPAD` is no longer added to `OracleDialect`, and
`CHAR_READ_SIDE_PADDING` is not supported there. `JDBCSuite` asserts Oracle
compiles neither. `OracleIntegrationSuite` now checks that CHAR equality stays
local and still returns the right rows. I did not add CJK data in that suite;
happy to follow up once someone can run Oracle docker.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2StrategySuite.scala:
##########
@@ -1882,6 +1883,42 @@ class DataSourceV2StrategySuite extends
SharedSparkSession {
}
}
+ test("SPARK-60479: read-side CHAR padding StaticInvoke translates to RPAD") {
+ val col = AttributeReference("flag", StringType)()
+ def paddingInvoke(functionName: String): StaticInvoke = {
Review Comment:
The unit test now builds the invoke via `CharVarcharUtils.addPaddingForScan`
on a `CHAR(1)` metadata attribute, and it has a negative case for
`readSidePadding` on a different class.
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -2350,13 +2350,25 @@ class JDBCSuite extends SharedSparkSession {
assert(getJdbcType(oracleDialect, TimestampNTZType) == "TIMESTAMP")
}
+ test("SPARK-60479: Oracle dialect compiles RPAD for CHAR filter pushdown") {
+ val oracleDialect = JdbcDialects.get("jdbc:oracle://127.0.0.1/db")
+ val rpad = new GeneralScalarExpression("RPAD",
+ Array[V2Expression](
+ FieldReference("PROD_VERSION_CURRENT"),
Review Comment:
That Oracle compile test is gone with the Oracle opt-out. The replacement
compile test uses `flag` on H2/Postgres/SQL Server.
--
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]
