cloud-fan commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4292171745
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -489,7 +566,9 @@ case class VariantGet(
path: Expression,
targetType: DataType,
failOnError: Boolean,
- timeZoneId: Option[String] = None)
+ timeZoneId: Option[String] = None,
+ legacyPathBackslashAsLiteral: Boolean =
Review Comment:
**Non-blocking (P2):** This field decides whether a backslash in a bracket
key is an escape, and Spark's own evaluation, `VariantMetadata` and
`ParquetFilters` all honor it. The unchanged `V2ExpressionBuilder` (lines
346-356), however, still turns every foldable-path `VariantGet` into the public
connector `VariantGet(child, path, targetType, failOnError, timeZoneId)`, which
has no mode and compares equal across modes. For `$['a\nb']`, legacy mode reads
the literal key `a\nb` and the default reads `a`+LF+`b`, yet both translate to
the same connector object.
That class (`@Evolving`, since 4.3.0) is documented as the connector-facing
form of `variant_get` and feeds DSv2 filter pushdown, metadata-only DELETE,
overwrite-by-filter, CHECK constraints and generated columns. A third-party
connector that reports such a predicate as fully handled, or accepts a
metadata-only delete, can silently filter or delete by a different key than
Spark's own evaluation in one of the two modes. `V2ExpressionBuilder` already
declines mode-dependent translations it cannot represent (`Cast` outside ANSI
or safe up-casts, `Abs` without `failOnError`). Spark's built-in sources are
not affected: `V2ExpressionSQLBuilder` rejects `VariantGet`, and Parquet reads
the mode from `VariantMetadata`.
**Recommended change:** In V2ExpressionBuilder, decline to translate a
VariantGet (return no connector expression) when it captured legacy mode and
its path contains a backslash inside a bracket-quoted key, a conservative
superset of the paths whose legacy and escaped parses can differ. Document on
the connector VariantGet that path() follows Spark's escaped JSONPath grammar
for bracket-quoted keys. Add DSv2 translation tests for both modes.
**Why this works:** Declining translation for mode-sensitive legacy paths
keeps those predicates and expressions in Spark, which evaluates them with the
captured mode. Connectors then only receive paths whose meaning matches the
documented grammar. This follows the existing V2ExpressionBuilder rule that
pushes Cast only for ANSI or safe up-casts and Abs only with failOnError.
**Scope:** Guard the DSv2 translation of variant_get on the captured parser
mode and document the connector-facing path grammar.
**Compatibility:** Default-mode DSv2 pushdown of variant_get predicates and
expressions.
**Risks:** In legacy mode, variant paths whose bracket keys contain
backslashes lose DSv2 pushdown, so such queries return more rows to Spark for
filtering and metadata-only deletes fall back to row-level rewrites.
**Constraints:** Default-mode translation of every variant path, and
legacy-mode translation of paths without backslashes in bracket keys, must stay
exactly as before, including boolean predicate wrapping.
**Exclusions:**
- Adding fields or accessors to the connector VariantGet class.
- Documenting the variant extraction metadata contract, which is a separate
connector surface.
**Success:** A VariantGet that captured legacy mode and whose bracket key
contains a backslash is not translated to a connector expression and is
evaluated by Spark. Default-mode VariantGet, and legacy-mode paths without such
keys, translate unchanged. The connector VariantGet documentation states the
path grammar connectors must apply.
**Verification:**
- **Behavior:** Translating a VariantGet whose bracket key contains a
backslash yields no connector expression when legacy mode was captured, and
yields the connector VariantGet with the raw path when the default mode was
captured.
- **Compatibility:** Translation of dot-notation and backslash-free bracket
paths, including predicate wrapping for boolean results, is unchanged in both
modes.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -369,17 +370,87 @@ object VariantPathParser extends RegexParsers {
override def skipWhitespace: Boolean = false
// Parse key segment like `.name`, `['name']`, or `["name"]`.
- private def key: Parser[VariantPathSegment] =
+ private def literalBackslashKey: Parser[VariantPathSegment] =
for {
key <- '.' ~> "[^\\.\\[]+".r | "['" ~> "[^']*".r <~ "']" |
"[\"" ~> """[^"]*""".r <~ "\"]"
} yield {
ObjectExtraction(key)
}
- private val parser: Parser[List[VariantPathSegment]] = phrase(root ~>
rep(key | index))
+ // Only bracket keys decode escapes.
+ private def escapedKey: Parser[VariantPathSegment] =
+ '.' ~> "[^\\.\\[]+".r ^^ ObjectExtraction |
+ "['" ~> """(?:\\[\s\S]|[^'\\])*+""".r <~ "']" ^^ { k =>
Review Comment:
**Nit (P3):** This grammar now decodes escapes, but colon extraction
validates and names its path with a different parser.
`AstBuilder.visitSemiStructuredExtract` builds `"$" + ctx.path.getText` and
still checks it and derives the column alias with `JsonPathParser`, whose
bracket rule (`['`, `[^']+`, `']`) is literal. The `VariantGet` it then builds
parses the same text here, so the two disagree:
- `SELECT v:['a\tb'] FROM t` names the column `a\tb` (backslash, t) but
reads the key `a`+TAB+`b`.
- `v:['a\'b']` lexes as one string literal but fails `JsonPathParser` with
`PARSE_SYNTAX_ERROR`, although `variant_get` now accepts the same path as the
key `a'b`.
Extracted values are otherwise correct, and the `variant_get` /
`try_variant_get` function forms are unaffected; only colon-syntax bracket keys
containing a backslash are involved.
**Recommended change:** In visitSemiStructuredExtract, validate the built
path and derive the alias with VariantPathParser, using the session's current
legacy flag (the value the resulting VariantGet will capture) and taking the
alias from the last object-key segment. Keep raising PARSE_SYNTAX_ERROR when
the path does not parse. Add colon-syntax SQL tests for bracket keys with
escapes in both modes.
**Why this works:** Validation and alias then come from the same grammar and
mode as execution: escaped quotes are accepted, and decoded keys name their own
columns.
**Scope:** Make colon-syntax extraction validate and name paths with the
Variant path grammar.
**Compatibility:** Results and aliases of colon-syntax paths without
backslashes in bracket keys.
**Risks:** VariantPathParser accepts some bracket forms JsonPathParser
rejected, such as double-quoted and empty keys, so colon syntax would start
accepting them; `['*']` would also gain the alias `*` (execution already reads
it as the literal key `*`).
**Constraints:** Colon paths without backslashes in bracket keys must keep
their current results and aliases. Parse-time validation must use the same
legacy flag value the analyzer captures, so validation and execution never
disagree.
**Exclusions:**
- Changing JsonPathParser, get_json_object, or JSON_TABLE path handling.
**Success:** In default mode, `v:['a\'b']` parses and extracts the key a'b.
`v:['a\tb']` produces a column named after the decoded key it extracts. In
legacy mode, colon-syntax bracket keys containing backslashes keep their
pre-4.4 literal results and aliases.
**Verification:**
- **Behavior:** Colon-syntax extraction of bracket keys containing escapes
returns the decoded key's value and names the column after that key in default
mode, and keeps literal-backslash semantics and names in legacy mode.
- **Compatibility:** Existing colon-syntax extractions without backslashes
keep their results, aliases and analyzed plans under both the fixed-point and
single-pass analyzers.
- **Regression:** In default mode, colon-syntax `v:['a\'b']` parses and
returns the value of key a'b instead of raising a syntax error.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -756,7 +789,69 @@ class VariantExpressionSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkInvalidPath("""$['"]""")
checkInvalidPath("$[\"\"\"]")
- checkInvalidPath("$[\"\\\"\"]")
+ }
+
+ test("legacy Variant paths treat backslashes literally") {
+ val input = """{"a\nb":1,"a\\nb":2}"""
+ val path = "$['a\\nb']"
+
+ withSQLConf(SQLConf.LEGACY_VARIANT_PATH_BACKSLASH_AS_LITERAL.key ->
"false") {
+ testVariantGet(input, path, IntegerType, 1)
+ }
+
+ withSQLConf(SQLConf.LEGACY_VARIANT_PATH_BACKSLASH_AS_LITERAL.key ->
"true") {
+ testVariantGet(input, path, IntegerType, 2)
+
+ val escapedQuote = """$['a\'b']"""
+ checkErrorInExpression[SparkRuntimeException](
+ variantGet("""{"a'b":1}""", escapedQuote, IntegerType),
+ "INVALID_VARIANT_GET_PATH",
+ Map("path" -> escapedQuote, "functionName" -> "`variant_get`"))
+ }
+ }
+
+ gridTest("Variant path functions use the captured parser mode")(Seq(false,
true)) { legacy =>
Review Comment:
**Nit (P3):** This grid flips the conf between construction and evaluation,
but each function gets only one path kind: `variant_delete`, `variant_pick`,
`variant_insert` and `variant_array_append` use constant paths, while
`variant_get` and `variant_set` use the dynamic `BoundReference`. So the
dynamic-path branches of the first four, the constant-path branch of
`variant_get`, and the flag that `setAtPath` / `arrayAppendAtPath` pass to
`renderVariantPath` are never checked under a flip (only `variant_insert`
diagnostics are). SQL `variant_delete` and `variant_pick` also go through the
new `def this(children: Seq[Expression])` constructors that `FunctionRegistry`
reflects on, and no test runs them with the legacy flag set. Hard-coding
`false` at any of these sites would keep every test green while the legacy
fallback the migration guide promises silently stops working for that function
or path kind.
**Verification:**
- **Regression:** For each of the six path functions, evaluation with both
constant and dynamic paths follows the mode captured at construction after the
session flag is flipped, in interpreted and generated code.
- **Regression:** variant_set and variant_array_append type-mismatch
failedAt prefixes follow the captured mode.
- **Behavior:** SQL variant_delete and variant_pick honor
spark.sql.legacy.variantPathBackslashAsLiteral=true for a key whose meaning
depends on the mode.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -47,23 +47,27 @@ case class VariantMetadata(
// with the offending value when the paired data field's strict cast raises
// INVALID_VARIANT_CAST. We pair by NAME (not struct ordinal) because
later pruning or
// reordering of struct fields preserves names but may shift positions.
- castErrorFor: Option[String] = None) {
+ castErrorFor: Option[String] = None,
+ // Path parser mode captured from the originating VariantGet.
+ legacyPathBackslashAsLiteral: Boolean = false) {
// Produce a metadata contain one key-value pair. The key is the special
`METADATA_KEY`.
- // The value contains key-value pairs for `path`, `failOnError`,
`timeZoneId`, and -- for
- // companion fields only -- `castErrorFor`.
+ // The value contains key-value pairs for `path`, `failOnError`,
`timeZoneId`,
+ // `legacyPathBackslashAsLiteral`, and -- for companion fields only --
`castErrorFor`.
def toMetadata: Metadata = {
val inner = new MetadataBuilder()
.putString(VariantMetadata.PATH_KEY, path)
.putBoolean(VariantMetadata.FAIL_ON_ERROR_KEY, failOnError)
.putString(VariantMetadata.TIME_ZONE_ID_KEY, timeZoneId)
+ .putBoolean(
Review Comment:
**Nit (P3):** This key now reaches connectors: `V2ScanRelationPushDown`
passes `field.path.toMetadata` to `VariantExtractionImpl`, and third-party
`SupportsPushDownVariantExtractions` implementations read it through
`VariantExtraction.metadata()`. That public Javadoc still lists only `path`,
`failOnError` and `timeZoneId` and says `path` follows JSON path syntax. A
connector written from it cannot know that this key switches `path` to the
literal-backslash reading, or that default-mode bracket keys decode escapes, so
for a key containing a backslash it can extract a different key than Spark.
Spark's built-in Parquet reader reads the key through `VariantMetadata` and is
unaffected; the gap is in the documented connector contract.
**Verification:**
- **Inspection:** The connector documentation for variant extraction
metadata names every key Spark writes for a pushed extraction and defines how
path is parsed when legacyPathBackslashAsLiteral is false and when it is true.
##########
docs/sql-migration-guide.md:
##########
@@ -28,6 +28,7 @@ license: |
## Upgrading from Spark SQL 4.3 to 4.4
+- Since Spark 4.4, recognized escape sequences in quoted Variant path keys are
decoded. In Spark 4.3 and earlier, backslashes were treated literally. Existing
paths may therefore match different keys or become invalid. For example,
`$['a\']` now raises an invalid-path error; use `$['a\\']` instead.
Unrecognized escapes remain literal, and dot-notation keys are unchanged. To
restore the previous behavior, set
`spark.sql.legacy.variantPathBackslashAsLiteral` to `true` before creating the
query.
Review Comment:
**Nit (P3):** This note asks users to audit existing paths, but its key term
is undefined: it never lists the recognized escapes, and the `variant_get` docs
only say the path is JSONPath. In the parser the set is `\\`, `\/`, `\b`, `\f`,
`\n`, `\r`, `\t`, the enclosing quote only (`\'` inside `['...']`, `\"` inside
`["..."]`), `\uXXXX` for non-surrogate code units, and surrogate pairs; lone
surrogates and every other escape stay literal. Without that list a user cannot
tell that `$['C:\new']` now reads `C:`+LF+`ew` while `$['C:\data']` keeps its
4.3 meaning, or that `$['x\/y']` now reads `x/y`.
**Verification:**
- **Inspection:** The 4.4 migration entry, or the reference it links, lists
exactly the escapes the Variant path parser decodes, including the
quote-specific and surrogate rules.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -756,7 +789,69 @@ class VariantExpressionSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkInvalidPath("""$['"]""")
checkInvalidPath("$[\"\"\"]")
- checkInvalidPath("$[\"\\\"\"]")
Review Comment:
**Nit (P3):** This was the suite's only negative case involving a backslash;
it is now valid by design, but nothing replaces it. No assertion covers the
boundaries the new grammar and the migration note define: `$['a\']` and
`$["a\"]` (now invalid, and the former is the guide's own example), an escaped
quote just before `]` (`$['a\']b']` must stay the single key `a']b`), or a
default-mode dot key with a backslash (`$.a\nb` must remain the literal key).
On the encoder side, the `failedAt` cases exercise only the backslash, the
active quote and `\n`; `quoteKey`'s `\b`, `\f`, `\r`, `\t` and generic `\u%04x`
branches never run. A regression in any of these would keep the suite green
while changing which keys existing paths address, or making a rendered
`failedAt` stop parsing back to the same key.
**Verification:**
- **Regression:** In default mode `$['a\']` and `$["a\"]` raise the
invalid-path error for variant_get and for the manipulation functions.
- **Behavior:** In default mode `$['a\']b']` addresses the single key a']b,
and `$.a\nb` addresses the literal key a\nb.
- **Regression:** failedAt for bracket-forced keys containing each short
control character, and another control character below 0x20, re-parses to the
same key.
--
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]
