cloud-fan commented on code in PR #60063:
URL: https://github.com/apache/spark/pull/60063#discussion_r4483400247
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala:
##########
@@ -652,12 +652,28 @@ class ExpressionParserSuite extends AnalysisTest {
assertEqual("`c`.a[b]", UnresolvedAttribute("c.a").getItem($"b"))
}
- test("invalid semi-structured extract path") {
- checkError(
- exception = parseException("c:['']"),
- condition = "PARSE_SYNTAX_ERROR",
- parameters = Map("error" -> "'['']'", "hint" -> ""),
- queryContext = Array(ExpectedContext("c:['']", 0, 5)))
+ test("semi-structured extract paths") {
Review Comment:
**Nit (P3):** Taking the alias from `VariantPathParser` also renames columns
whose last bracket key is `'*'`. On the base branch `JsonPathParser` read
`['*']` as a wildcard, so `c:a['*']` was named `a` and `c:['*']` got no
inferred name; now both are named `*` under either flag value. This test covers
empty and escaped keys but not `['*']`, and no other changed test does, so a
return to wildcard naming would still pass. A case such as
`assertEqual("c:a['*']", Alias(SemiStructuredExtract($"c", "$.a['*']"),
"*")())`, plus the root form `c:['*']`, would pin it.
**Verification:**
- **Regression:** Colon paths whose last bracket key is '*', both after a
dot key and at the root, parse to an extraction named `*` under both values of
the legacy flag.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -4774,14 +4773,15 @@ class AstBuilder extends DataTypeAstBuilder
// When `field` starts with a bracket, do not add a `.` as the bracket
already implies nesting
// Also the bracket will imply case sensitive field extraction.
val path = if (field.startsWith("[")) "$" + field else s"$$.$field"
- val parsedPath = JsonPathParser.parse(path)
+ val parsedPath = VariantPathParser.parse(
+ path, conf.getConf(SQLConf.LEGACY_VARIANT_PATH_BACKSLASH_AS_LITERAL))
Review Comment:
**Non-blocking (P2):** The flag is read here, at parse time, to validate the
path and choose the inferred column name, but `SemiStructuredExtract` keeps
only the path text, and both analyzers later build `VariantGet`, whose
`legacyPathBackslashAsLiteral` default reads the conf again. If the flag
changes between parsing and analysis of the same plan, the name and the
extracted key follow different modes.
Spark Connect reaches this. The server caches a DataFrame's unanalyzed plan
keyed by its relation (`SessionHolder.usePlanCache`), and a reused PySpark
DataFrame resends the same plan ID. From reading the code: if `df =
vdf.selectExpr("v:['a\\nb']")` is collected in a default session, then
`spark.conf.set("spark.sql.legacy.variantPathBackslashAsLiteral", "true")` runs
and `df` is collected again, the column keeps the name decoded at parse time
(a, newline, b), while `VariantGet` now extracts the literal four-character key
`a\nb`. On the base branch the name was always the raw text `a\nb`, so it
matched the literal extraction and a by-name reference to it worked. The
opposite order keeps the base branch's raw name. The extracted values are the
same as on the base branch; the problem is a column name that no longer matches
them.
**Recommended change:** Record on the colon-path node the Variant path mode
that the parser already reads, and build the extraction with that recorded mode
in both the fixed-point and the single-pass analyzer, so a colon path's
validity, inferred column name and extracted key always use the same mode; add
parser or analysis tests that change the flag between parsing and analysis.
**Why this works:** The parser stores the flag value it passed to
VariantPathParser.parse on SemiStructuredExtract, and both analyzer conversions
pass that value to VariantGet's legacyPathBackslashAsLiteral parameter instead
of letting VariantGet read the conf again, so a later conf change can no longer
split the name from the extraction.
**Scope:** Carry a colon path's parse-time Variant path mode from the parser
through both analyzers to the VariantGet that extracts it.
**Compatibility:** Persisted views and ordinary SQL and DataFrame queries,
which parse and analyze under one conf, keep their current column names,
extracted values and errors.
**Risks:** A colon path parsed before
spark.sql.legacy.variantPathBackslashAsLiteral changes, such as one in a Spark
Connect DataFrame run again after spark.conf.set, keeps extracting with the
mode it was parsed under, where the base branch used the new value. A
variant_get call in the same DataFrame is built only during analysis and
follows the new value, so in that sequence the two can read different keys for
the same path. This matches how a parser-built CAST keeps the ANSI mode it was
parsed under, and the migration guide's instruction to set the flag before
creating the query.
**Constraints:** The parse-time PARSE_SYNTAX_ERROR for rejected colon paths
keeps its error class, parameters and query context. The fixed-point and
single-pass analyzers both use the recorded mode, so they keep producing the
same extraction for the same colon path.
**Exclusions:**
- Moving inferred-name derivation from the parser to analysis is out of
scope; the parser keeps deriving the name, as the PR intends.
**Success:** For every colon path, the inferred name, the parse-time
validity and the key the extraction reads follow one parser mode: the value of
the flag when the path was parsed. Changing the flag after a colon path is
parsed and before it is analyzed no longer produces a column named after one
key while holding another key's value. Queries that parse and analyze under one
flag value, including persisted views, produce the same names, values and
errors as before the change.
**Verification:**
- **Regression:** Parse an unaliased colon path whose bracket key contains a
backslash escape with the flag off, analyze and run it with the flag on, and
check that the column name and the extracted value both come from the decoded
key; then reverse the order and check that both come from the literal key.
- **Compatibility:** With the flag held at either value through parsing and
analysis, colon paths keep the names, values and parse errors the existing
colon-path tests expect, under both the fixed-point and the single-pass
analyzer.
--
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]
