cloud-fan commented on code in PR #59820:
URL: https://github.com/apache/spark/pull/59820#discussion_r4400926113
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ResolveDefaultColumnsUtil.scala:
##########
@@ -351,6 +359,9 @@ object ResolveDefaultColumns extends QueryErrorsBase
expr match {
case _: ExprLiteral => expr
case c: Cast if c.resolved => expr
+ case c @ Collate(_: ExprLiteral, _: UnresolvedCollation) =>
Review Comment:
**Non-blocking (P2):** This match keeps the escape-processed parse only for
a top-level literal, a resolved cast, or this collated literal. `Literal.sql`
writes ARRAY, MAP and STRUCT values as `ARRAY(...)`, `MAP(...)` and
`NAMED_STRUCT(...)`, escaping each nested string the same way as a top-level
one (literals.scala:720-744). Those calls parse to `CreateArray`, `CreateMap`
or `CreateNamedStruct` and fall to `case _`.
`fallbackResolveExistenceDefaultValue` then reparses the text with `new
CatalystSqlParser()` under the session's `escapedStringLiterals`.
So with `spark.sql.parser.escapedStringLiterals=true`, the problem this PR
fixes for STRING columns remains for complex columns. Take `ALTER TABLE ... ADD
COLUMNS (s STRUCT<y: STRING> DEFAULT ...)`: pre-existing rows read nested
strings with an extra backslash before each backslash and single quote, while
rows inserted later with `DEFAULT` get the original value. This applies to
nested collated strings too, and to every file reader that uses
`existenceDefaultValues`. Default mode is unaffected.
I confirmed this on this head with a decoder-level repro. A nested value of
`a\b'c`, stored as `Literal.sql` text and read back after a schema JSON round
trip, came back as `a\\b\'c` in escaped mode for ARRAY, STRUCT and MAP. A
top-level STRING default with the same value decoded correctly, as did every
type in default mode. The producer is supported: the existing InsertSuite test
that adds a `struct<x boolean, y string>` column with a default exercises it.
**Recommended change:** Treat an existence default whose escape-processed
parse is a tree of ARRAY, MAP or NAMED_STRUCT constructors with only literal
leaves as Literal.sql output, and decode it from that parse instead of the
session-parsed fallback. Literal leaves are a Literal, a Cast of a Literal, or
a collated Literal. Make CatalogV2Util.structFieldToV2Column build its
ColumnDefaultValue literal from this same decoder, for example through the
existing ResolveDefaultColumns.getExistenceDefaultValue or a Literal-level
method, instead of analyze(f, "Column analysis",
EXISTS_DEFAULT_COLUMN_METADATA_KEY). Add existence-default tests for complex
types with nested strings and for the V2 column conversion in both parser
modes, at the decoder level and through a file-source table with existing rows.
**Why this works:** A small recursive predicate in the decoder accepts
exactly the shapes Literal.sql emits for complex values: CreateArray, CreateMap
and CreateNamedStruct whose children are themselves accepted constructors or
literal leaves. A matching expression goes through the same default-column
analysis helper the new collated-literal arm already uses. That helper resolves
nested collations, constant-folds the constructor and coerces it to the column
type without reparsing the text, so nested strings keep the values the
escape-processed parse produced. Complex-type shapes the predicate rejects, and
any other shape the existing arms do not accept, still reach the existing
session-parsed fallback. The V2 conversion then evaluates the existence default
through this decoder, as getExistenceDefaultValue already does for the readers,
so the V2 literal and the readers' value come from one decoding.
**Scope:** Extend the reader decoder's canonical-shape recognition to
literal-only complex constructors, route the V2 column conversion through the
same decoder, and add complex-type and V2-conversion existence-default coverage
in both parser modes.
**Compatibility:** Session-based analysis, including session string-literal
parsing, for every existence default that reaches the unresolved-expression
fallback today, including complex-type text with a function call at any level;
the PR's handling of top-level literal, resolved-cast and collated-literal
defaults; the SQLConf identity and settings during decoding; and the V2
conversion's CURRENT_DEFAULT SQL text, which passes through unchanged.
**Risks:** Literal.sql writes NAMED_STRUCT field names without escaping, so
a field name that contains a backslash is now unescaped in escaped mode, as it
already is in default mode. The value is still assigned to the column by
position through default-value coercion. A field name with a single quote makes
the parse fail, and the decoder keeps falling back as it does today. The V2
conversion now reports an unresolvable existence default through the reader
decoder's error path, with an empty statement type instead of "Column
analysis", unless the shared entry point passes the statement type through. It
also logs the unresolved-existence-default fallback warning for text that is
not Literal.sql output. Values are unchanged in default mode.
**Constraints:** Leave the existing top-level resolved-cast arm unchanged.
No in-repo writer stores cast-wrapped function text, and this review assumes no
writer outside the repository does. Never copy or modify the active SQLConf,
and keep the session time zone and other settings in effect during analysis of
the canonical parse.
**Exclusions:**
- Literal.sql's own rendering of NAMED_STRUCT field names and PARSE_JSON
payloads predates this PR and is left unchanged.
**Success:** With spark.sql.parser.escapedStringLiterals set to true or
false, a row written before an ARRAY, MAP or STRUCT column with a
string-bearing default was added reads the same nested values as a row inserted
later with DEFAULT, including strings with backslashes, single quotes and
collations. Existence defaults whose text is a Literal.sql complex-type
constructor are decoded from the canonical parse and no longer log the
unresolved-existence-default fallback warning. Complex-type existence-default
text that contains a function call at any level still decodes with the
session's string-literal parsing. With spark.sql.parser.escapedStringLiterals
set to true or false, CatalogV2Util.structTypeToV2Columns builds each column's
ColumnDefaultValue literal with the same value that existenceDefaultValues
returns for that field, for plain, collated and nested strings, and converting
the columns back with v2ColumnsToStructType preserves that value.
**Verification:**
- **Regression:** In escaped mode, a file-source table with an existing row
and newly added ARRAY of STRING, MAP of STRING to STRING, and STRUCT with a
STRING field columns, whose defaults contain a backslash and a single quote,
returns the same values for the old row as for a row inserted with DEFAULT,
with both the vectorized and the row-based Parquet reader.
- **Regression:** In escaped mode, CatalogV2Util.structTypeToV2Columns on a
schema that has gone through a JSON round trip returns a ColumnDefaultValue
literal equal to existenceDefaultValues for STRING, collated STRING and
complex-type existence defaults whose strings contain a backslash and a single
quote. Converting the columns back with v2ColumnsToStructType and decoding
again returns the same value, and CREATE TABLE ... LIKE from a V1 table into an
in-memory V2 catalog passes the target catalog that value.
- **Behavior:** In both parser modes, decoding a schema that has gone
through a JSON round trip returns the original value for complex-type existence
defaults with plain and collated nested strings, null elements, empty strings,
and nested constructors such as an array of structs or a map whose values are
arrays.
- **Compatibility:** In escaped mode, a complex-type existence default whose
text contains a function call, such as an ARRAY over a concat of string
literals with a backslash escape, still decodes with session string-literal
parsing. Default-mode results stay unchanged for every shape.
--
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]
