HyukjinKwon commented on code in PR #59302:
URL: https://github.com/apache/spark/pull/59302#discussion_r4236877230
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -1856,6 +1858,26 @@ class AstBuilder extends DataTypeAstBuilder
schemaLess
)
)
+ // First-class CHAR/VARCHAR (standard semantics or
preserveCharVarcharTypeInfo):
+ // assign declared types in a Project so the result matches AS (...). Flag
off keeps
+ // the historical STRING result with no pad and no EXCEED_LIMIT_LENGTH.
The check can
+ // be pruned like scan-side assignment when the column is unreferenced.
+ if (conf.charVarcharFirstClassTypes &&
+ schema.exists(f => CharVarcharUtils.hasCharVarchar(f.dataType))) {
+ Project(
+ attributes.zip(schema).map { case (a, field) =>
+ if (CharVarcharUtils.hasCharVarchar(field.dataType)) {
+ Alias(
+ CharVarcharUtils.stringLengthCheck(a, field.dataType),
+ a.name)(explicitMetadata =
Some(CharVarcharUtils.cleanMetadata(a.metadata)))
+ } else {
+ a
Review Comment:
**Nit (P3):** This passthrough branch, and the positional
`attributes.zip(schema)` pairing around it, has no test that would fail if it
broke. Every first-class TRANSFORM test in `BaseScriptTransformationSuite`,
`SparkScriptTransformationSuite`, and `HiveScriptTransformationSuite` declares
only columns that contain CHAR/VARCHAR. The typed tests with other column types
run with first-class types off, where no Project is built. Mixed declarations
such as `AS (id INT, name CHAR(4), note STRING)` are the common shape, so a
later change that drops, reorders, or retypes a non-CHAR column here would ship
unnoticed. The code is correct today. One mixed-column case in
`BaseScriptTransformationSuite`, which runs on both the no-SerDe and Hive SerDe
paths, asserting the full schema and the row values would close this.
**Verification:**
- **Regression:** Dropping, reordering, retyping, or misaligning a non-CHAR
column in the assignment Project makes the mixed-column assertion fail.
- **Behavior:** A mixed declaration returns the declared types in order,
with only the CHAR/VARCHAR columns padded or length-checked.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/SparkScriptTransformationSuite.scala:
##########
@@ -62,38 +207,33 @@ class SparkScriptTransformationSuite extends
BaseScriptTransformationSuite with
|SELECT TRANSFORM('abcdef') USING 'cat' AS (c CHAR(4))
|FROM VALUES (1) input(dummy)""".stripMargin)
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "false") {
- val exception = intercept[Exception] {
- sql("SELECT * FROM v_overflow").collect()
- }
- val runtimeException = exception match {
- case s: org.apache.spark.SparkRuntimeException => s
- case other =>
-
other.getCause.asInstanceOf[org.apache.spark.SparkRuntimeException]
- }
- checkError(
- exception = runtimeException,
- condition = "EXCEED_LIMIT_LENGTH",
- parameters = Map("limit" -> "4"))
+ checkExceedLimitLength(
+ intercept[Exception](sql("SELECT * FROM v_overflow").collect()),
+ "4")
}
}
}
- withSQLConf(
- SQLConf.PRESERVE_CHAR_VARCHAR_TYPE_INFO.key -> "true",
- SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "false") {
+ assert(uncaughtExceptionHandler.exception.isEmpty)
+ }
+
+ test("SPARK-60090: TRANSFORM view created at default follows the caller
session flag") {
+ assume(TestUtils.testCommandAvailable("/bin/bash"))
+ // PERSISTED stores the create-time flag when it is set true (see the test
above).
+ // The default false is not stored on the view, so a later session with
the flag on
Review Comment:
**Nit (P3):** This comment says the flag-off view is re-parsed under the
caller's flag and gets assignment, but PERSISTED configs do not bind that way.
`SessionCatalog.fromCatalogTable` parses the view text under
`View.effectiveSQLConf`, which starts from the view's stored configs and copies
from the caller only session-binding-policy keys.
`spark.sql.charVarchar.standardSemantics.enabled` is `PERSISTED`, so
`v_disabled` is parsed with the flag off and has no assignment Project. The
`CHAR(4)` / `'ab '` result comes from `castColToType` casting the view column
to its stored `CHAR(4)` schema. The assertions still pass, but the test name
and comment describe a mechanism that does not run. As a result, overflow
through such a view (cast semantics, not the Project's `EXCEED_LIMIT_LENGTH`)
looks covered when it is not. This only affects views created with the flag
off; the flag-on view test above is accurate. Could you rename this test and
reword the comment to describe the stored-flag
parse plus the view-output cast?
**Verification:**
- **Inspection:** The test name and comment no longer claim the caller
session's flag applies when the view is re-parsed, and agree with how
View.effectiveSQLConf binds PERSISTED configs.
- **Behavior:** Read with standard semantics on, the default-created view
still reports CHAR(4) and returns the padded value.
--
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]
