spark-test commented on code in PR #60059:
URL: https://github.com/apache/spark/pull/60059#discussion_r4471060884
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala:
##########
@@ -463,6 +463,13 @@ case class OuterReference(e: NamedExpression)
extends LeafExpression with NamedExpression with Unevaluable {
override def dataType: DataType = e.dataType
override def nullable: Boolean = e.nullable
+ // Forward only the raw CHAR/VARCHAR type when aliased. Other keys are not
forwarded; some,
+ // such as the SQL UDF parameter and auto-generated alias markers, must not
leak.
+ override def metadata: Metadata =
CharVarcharUtils.getRawTypeString(e.metadata) match {
Review Comment:
**Non-blocking (P2):** The forwarded type only takes effect when a query is
analyzed, so permanent views created before the upgrade don't pick it up.
`SessionCatalog.castColToType` aliases every view column with the stored
field's metadata (`explicitMetadata = Some(toField.metadata)`), and only `WITH
SCHEMA EVOLUTION` views expose the re-analyzed query.
`ViewSyncSchemaToMetaStore` compares only type and nullability, so it never
restores the missing key.
For example, a view created by an earlier version as `CREATE VIEW v AS
SELECT t.s, l.k FROM t, LATERAL (SELECT t.c AS k) l` over `c CHAR(5)` stored
`k` as plain STRING. After the upgrade, `SELECT s FROM v WHERE k = 'ab'` still
returns no rows, while the same query without the view now returns `x`. This
holds for all three forms (`t.c AS k`, `t.c`, `t.*`), because inside a LATERAL
subquery each of them lost the CHAR type in earlier versions. Only CHAR columns
are affected, since VARCHAR comparisons aren't padded.
The migration note says these comparisons now return the correct rows, but
doesn't say that such views must be recreated with `CREATE OR REPLACE VIEW` or
`ALTER VIEW ... AS`. Fixing them automatically would need a stored-schema
migration. Instead, the note should name this exemption and the recreate step,
with a TODO at the view column projection in `castColToType`, where the stored
metadata wins.
**Verification:**
- **Behavior:** A permanent view over `SELECT t.s, l.k FROM t, LATERAL
(SELECT t.c AS k) l`, with c CHAR(5) holding 'ab', records k as char(5) in its
stored schema, and `SELECT s FROM v WHERE k = 'ab'` returns the matching row.
- **Inspection:** The migration-guide entry and the TODO both name the
exemption for views created by earlier versions, and the step that recreates
them.
##########
docs/sql-migration-guide.md:
##########
@@ -52,6 +52,7 @@ license: |
- Since Spark 4.4, the Oracle JDBC datasource maps bare `NUMBER` columns
(declared without explicit precision or scale) to `DecimalType(38, 18)` instead
of `DecimalType(38, 10)`. This uses `DecimalType.DEFAULT_SCALE` and preserves
up to 18 fractional digits instead of 10, reducing silent precision loss.
However, the integer range shrinks from 28 to 20 digits; bare `NUMBER` values
with more than 20 integer digits that previously read correctly will raise
`NUMERIC_VALUE_OUT_OF_RANGE`. The same change applies to Oracle `FLOAT`
columns, which JDBC reports as `NUMERIC` with `scale = -127`. Columns with
explicit `NUMBER(p,s)` are not affected. To restore the previous
`DecimalType(38, 10)` mapping, set
`spark.sql.legacy.oracle.numberMapping.enabled` to `true`.
- Since Spark 4.4, when `spark.sql.charVarchar.standardSemantics.enabled` is
false and `spark.sql.readSideCharPadding` is true (the default), JDBC Data
Source V2 catalog tables on H2, PostgreSQL, SQL Server, and Oracle push CHAR
equality, inequality, null-safe equality, and `IN`/`NOT IN`. In 4.3 the default
read-side padding wrap kept those filters in Spark. Pushed predicates use the
remote collation, so a case-insensitive SQL Server collation can match a stored
`'ABC'` with `flag = 'abc'` while a Spark-local comparison would not. The V1
path (`spark.read.jdbc` and `USING jdbc`) still evaluates these CHAR filters in
Spark. PostgreSQL's internal `"char"` type (JDBC type name `char`) is mapped to
`STRING` instead of `CHAR(1)`, so Spark no longer blank-pads its empty value.
- Since Spark 4.4, when `spark.sql.charVarchar.standardSemantics.enabled` is
false and `spark.sql.readSideCharPadding` is true (the default), `IS [NOT]
NULL` over a CHAR scan column is unwrapped to the raw column for every Data
Source V2 source, including every JDBC dialect. Previously the padding wrap
kept that null check in Spark.
+- Since Spark 4.4, a CHAR/VARCHAR column of the outer query that a LATERAL or
correlated subquery returns keeps its CHAR/VARCHAR type, as an inner column
does. Previously, comparisons against it were not padded and could silently
return no rows, and CTAS and views over it stored `STRING`. This affected `t.c
AS k` in all earlier versions, `t.*` since Spark 4.2, and `t.c` since Spark
4.3. To keep storing `STRING`, set `spark.sql.legacy.charVarcharAsString` to
`true`, which treats all CHAR/VARCHAR columns as `STRING`.
Review Comment:
**Nit (P3):** The version history is right for correlated subqueries outside
LATERAL, but not inside a LATERAL subquery, the first case this entry names.
SPARK-55794 (4.2.0) started aliasing outer star expansion, and SPARK-58171
(4.3.0) bare outer references. Before that, a LATERAL `SELECT t1.c` or `SELECT
t1.*` exposed the outer attribute's own ExprId, which collided with the left
side. `DeduplicateRelations` resolved the collision by wrapping the reference
in `Alias(o, a.name)()` (`DeduplicateRelations.scala:399-409`). Before this PR
`OuterReference` had empty metadata, so that alias dropped the CHAR type too.
`ResolveSubquerySuite`'s `deduplicate lateral subquery` test has asserted that
alias since 2021, and neither ticket touched `DeduplicateRelations`.
So on 4.2.0, `SELECT t1.s FROM t1, LATERAL (SELECT t1.c) l WHERE l.c = 'ab'`
already returns no rows, and `LATERAL (SELECT t1.*)` behaves the same before
4.2. The sentence should say that in a LATERAL subquery all three forms were
affected in all earlier versions, and keep the 4.2 and 4.3 dates for other
correlated subqueries.
**Verification:**
- **Inspection:** The entry no longer dates the breakage of the bare and
star forms in a LATERAL subquery to Spark 4.3 and 4.2, and still gives those
dates for other correlated subqueries.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -866,6 +866,53 @@ trait CharVarcharTestSuite extends QueryTest {
}
}
+ test("SPARK-XXXXX: char type comparison: outer reference returned by a
LATERAL subquery") {
Review Comment:
**Nit (P3):** This test name still carries the `SPARK-XXXXX` placeholder
instead of this PR's key, `SPARK-60876`. So do the test at line 893 and the new
`PersistedViewTestSuite` test at `SQLViewTestSuite.scala:630`. Neighboring
regression tests carry their real keys (for example `SPARK-34833` here). With
the placeholder, these tests can't be traced back to their ticket or selected
by its ID after merge.
**Verification:**
- **Inspection:** Every test name this change adds begins with SPARK-60876,
and none contains the SPARK-XXXXX placeholder.
##########
docs/sql-migration-guide.md:
##########
@@ -52,6 +52,7 @@ license: |
- Since Spark 4.4, the Oracle JDBC datasource maps bare `NUMBER` columns
(declared without explicit precision or scale) to `DecimalType(38, 18)` instead
of `DecimalType(38, 10)`. This uses `DecimalType.DEFAULT_SCALE` and preserves
up to 18 fractional digits instead of 10, reducing silent precision loss.
However, the integer range shrinks from 28 to 20 digits; bare `NUMBER` values
with more than 20 integer digits that previously read correctly will raise
`NUMERIC_VALUE_OUT_OF_RANGE`. The same change applies to Oracle `FLOAT`
columns, which JDBC reports as `NUMERIC` with `scale = -127`. Columns with
explicit `NUMBER(p,s)` are not affected. To restore the previous
`DecimalType(38, 10)` mapping, set
`spark.sql.legacy.oracle.numberMapping.enabled` to `true`.
- Since Spark 4.4, when `spark.sql.charVarchar.standardSemantics.enabled` is
false and `spark.sql.readSideCharPadding` is true (the default), JDBC Data
Source V2 catalog tables on H2, PostgreSQL, SQL Server, and Oracle push CHAR
equality, inequality, null-safe equality, and `IN`/`NOT IN`. In 4.3 the default
read-side padding wrap kept those filters in Spark. Pushed predicates use the
remote collation, so a case-insensitive SQL Server collation can match a stored
`'ABC'` with `flag = 'abc'` while a Spark-local comparison would not. The V1
path (`spark.read.jdbc` and `USING jdbc`) still evaluates these CHAR filters in
Spark. PostgreSQL's internal `"char"` type (JDBC type name `char`) is mapped to
`STRING` instead of `CHAR(1)`, so Spark no longer blank-pads its empty value.
- Since Spark 4.4, when `spark.sql.charVarchar.standardSemantics.enabled` is
false and `spark.sql.readSideCharPadding` is true (the default), `IS [NOT]
NULL` over a CHAR scan column is unwrapped to the raw column for every Data
Source V2 source, including every JDBC dialect. Previously the padding wrap
kept that null check in Spark.
+- Since Spark 4.4, a CHAR/VARCHAR column of the outer query that a LATERAL or
correlated subquery returns keeps its CHAR/VARCHAR type, as an inner column
does. Previously, comparisons against it were not padded and could silently
return no rows, and CTAS and views over it stored `STRING`. This affected `t.c
AS k` in all earlier versions, `t.*` since Spark 4.2, and `t.c` since Spark
4.3. To keep storing `STRING`, set `spark.sql.legacy.charVarcharAsString` to
`true`, which treats all CHAR/VARCHAR columns as `STRING`.
Review Comment:
**Non-blocking (P2):** The rollback sentence doesn't hold:
`spark.sql.legacy.charVarcharAsString=true` doesn't keep STRING storage.
Relation attributes carry the CHAR/VARCHAR type key whether or not the flag is
set. Under the flag, `ApplyCharTypePadding` returns the plan unchanged, so
nothing strips the key, and the new `OuterReference.metadata` forwards it into
the alias. Then `CharVarcharUtils.getRawSchema`, which never reads this flag,
rebuilds `CHAR(n)`/`VARCHAR(n)` for data-source, Hive and v2 CTAS and for views.
So `SET spark.sql.legacy.charVarcharAsString=true; CREATE TABLE t2 USING
parquet AS SELECT l.k FROM t, LATERAL (SELECT t.c AS k) l` still stores `t2.k`
as `char(5)`, while the flag turns off CHAR padding and length checks for every
column in the session. Inner-column CTAS already behaves the same way, so the
flag was never a storage rollback.
What does keep STRING is a cast in the query, for example `CAST(t.c AS
STRING) AS k`: an `Alias` over a `Cast` inherits no metadata, and
`SimplifyCasts` keeps that cast. The sentence could point to that instead.
**Verification:**
- **Behavior:** A CTAS table and a permanent view built over a LATERAL
subquery that returns an outer CHAR(5) column through an explicit CAST to
STRING store that column as STRING, while the same statements without the cast
store char(5).
- **Inspection:** The migration-guide entry no longer presents
spark.sql.legacy.charVarcharAsString as a way to keep STRING storage.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala:
##########
@@ -463,6 +463,13 @@ case class OuterReference(e: NamedExpression)
extends LeafExpression with NamedExpression with Unevaluable {
override def dataType: DataType = e.dataType
override def nullable: Boolean = e.nullable
+ // Forward only the raw CHAR/VARCHAR type when aliased. Other keys are not
forwarded; some,
+ // such as the SQL UDF parameter and auto-generated alias markers, must not
leak.
+ override def metadata: Metadata =
CharVarcharUtils.getRawTypeString(e.metadata) match {
Review Comment:
**Nit (P3):** Since this is a `def`, every call builds a new
`MetadataBuilder` and a one-entry `Metadata` when the outer column is
CHAR/VARCHAR. `Alias.metadata` reads it on each `Alias.toAttribute`, and
`Project.output`, also a `def`, calls that on every evaluation. So the same
value is rebuilt many times during analysis and optimization, until
decorrelation removes the outer reference. Before this PR, the call returned
the shared `Metadata.empty`. Earlier in this file, the alias metadata path is
documented as performance-sensitive: `removeNonInheritableMetadata` avoids
`MetadataBuilder` for that reason. The value depends only on the immutable `e`,
so `override lazy val metadata` would compute it once with the same result. The
cost is planning-time only and probably small.
**Verification:**
- **Behavior:** Comparisons, result types and marker behavior for outer CHAR
and VARCHAR columns returned by LATERAL and correlated subqueries are unchanged
with the cached value.
- **Inspection:** Repeated metadata reads on one OuterReference instance no
longer construct a new MetadataBuilder.
--
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]
