You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "bojana-db (via GitHub)" <gi...@apache.org> on 2026/09/28 11:28:03 UTC
[PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
bojana-db opened a new pull request, #59097:
URL: https://github.com/apache/spark/pull/59097
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'common/utils/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
VariantPathParser now decodes escape sequences in the single-quoted ['...'] bracket form — \', \\, the short control escapes \b \t \n \f \r, and \uXXXX — so object-key paths emitted by recursive variant_explode round-trip back through variant_get. A backslash that doesn't begin a recognized escape is kept literal (so existing paths with literal backslashes keep working), and the dot notation and double-quoted ["..."] form are unchanged. This is a follow-up to review feedback on #57827 (SPARK-58645), covered by round-trip tests in VariantExpressionSuite.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4297949335
##########
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:
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59097:
URL: https://github.com/apache/spark/pull/59097#issuecomment-6139591517
@bojana-db can you fix merge conflicts?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4303146676
##########
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:
I would address this as a followup since it also requires making colon variation use `VariantPathParser`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4130213997
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -737,6 +737,23 @@ class VariantExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
// scalastyle:on nonascii
testVariantGet("[1, 2, 3]", "$[2147483647]", IntegerType, null)
+ // `['...']` decodes escapes: `\'` -> quote, `"` literal, both quotes in one key.
+ testVariantGet("""{"a'b": 1}""", """$['a\'b']""", IntegerType, 1)
+ testVariantGet("""{"a\"b": 2}""", """$['a"b']""", IntegerType, 2)
+ testVariantGet("""{"a'\"b": 3}""", """$['a\'"b']""", IntegerType, 3)
+ // `\\` -> backslash, and the short control escapes.
Review Comment:
**Non-blocking (P2):** This block claims the short control escapes, but it asserts only `\t` and `\n`. The decoder has independent mappings for `\b`, `\f`, and `\r`, so a typo in any of those branches would leave the suite green. Please add lookup assertions whose expected keys contain backspace, form feed, and carriage return.
**Verification:**
- **Behavior:** For each recognized short-control escape, look up an object key containing the corresponding decoded character and assert the expected value.
- **Regression:** Ensure that substituting one short-control mapping for another would fail at least one assertion.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -368,14 +369,35 @@ object VariantPathParser extends RegexParsers {
override def skipWhitespace: Boolean = false
- // Parse key segment like `.name`, `['name']`, or `["name"]`.
+ // Parse a key segment: `.name`, `['name']`, or `["name"]`. Dot notation is taken literally;
+ // both bracket forms are unescaped by `unescapeQuotedKey`.
private def key: Parser[VariantPathSegment] =
- for {
- key <- '.' ~> "[^\\.\\[]+".r | "['" ~> "[^']*".r <~ "']" |
- "[\"" ~> """[^"]*""".r <~ "\"]"
- } yield {
- ObjectExtraction(key)
- }
+ '.' ~> "[^\\.\\[]+".r ^^ ObjectExtraction |
+ "['" ~> """(?:\\[\s\S]|[^'\\])*""".r <~ "']" ^^ { k =>
+ ObjectExtraction(unescapeQuotedKey(k))
Review Comment:
**Non-blocking (P2):** These decoded keys also flow into `VariantExpressionEvalUtils.renderVariantPath` when a mutation fails at a later segment, but that renderer inserts object keys without escaping the selected quote delimiter or backslashes. A key containing both quote characters therefore produces a malformed `failedAt`, and a literal backslash followed by a recognized escape letter can be reported as a different key. The structured error should preserve the exact parsed prefix for every key admitted here.
**Verification:**
- **Regression:** Exercise a manipulation type mismatch beneath a key containing both quote delimiters and assert that failedAt is a valid equivalent path.
- **Regression:** Exercise a manipulation type mismatch beneath a bracket-forced key containing a literal backslash plus a recognized escape letter and assert that failedAt reparses to that literal key.
- **Compatibility:** Confirm that ordinary identifier keys, dotted keys, indices, and existing error prefixes retain their current representation and meaning.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4249909084
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -368,14 +369,35 @@ object VariantPathParser extends RegexParsers {
override def skipWhitespace: Boolean = false
- // Parse key segment like `.name`, `['name']`, or `["name"]`.
+ // Parse a key segment: `.name`, `['name']`, or `["name"]`. Dot notation is taken literally;
+ // both bracket forms are unescaped by `unescapeQuotedKey`.
private def key: Parser[VariantPathSegment] =
- for {
- key <- '.' ~> "[^\\.\\[]+".r | "['" ~> "[^']*".r <~ "']" |
- "[\"" ~> """[^"]*""".r <~ "\"]"
- } yield {
- ObjectExtraction(key)
- }
+ '.' ~> "[^\\.\\[]+".r ^^ ObjectExtraction |
+ "['" ~> """(?:\\[\s\S]|[^'\\])*""".r <~ "']" ^^ { k =>
+ ObjectExtraction(unescapeQuotedKey(k))
Review Comment:
Fixed.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -737,6 +737,23 @@ class VariantExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
// scalastyle:on nonascii
testVariantGet("[1, 2, 3]", "$[2147483647]", IntegerType, null)
+ // `['...']` decodes escapes: `\'` -> quote, `"` literal, both quotes in one key.
+ testVariantGet("""{"a'b": 1}""", """$['a\'b']""", IntegerType, 1)
+ testVariantGet("""{"a\"b": 2}""", """$['a"b']""", IntegerType, 2)
+ testVariantGet("""{"a'\"b": 3}""", """$['a\'"b']""", IntegerType, 3)
+ // `\\` -> backslash, and the short control escapes.
Review Comment:
Added.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4340959296
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -737,6 +737,41 @@ class VariantExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
// scalastyle:on nonascii
testVariantGet("[1, 2, 3]", "$[2147483647]", IntegerType, null)
+ testVariantGet("""{"a'b": 1}""", """$['a\'b']""", IntegerType, 1)
+ testVariantGet("""{"a']b": 1}""", """$['a\']b']""", IntegerType, 1)
+ testVariantGet("""{"a\nb":1,"a\\nb":2}""", """$.a\nb""", IntegerType, 2)
+ testVariantGet("""{"a\"b": 2}""", """$['a"b']""", IntegerType, 2)
+ testVariantGet("""{"a\\b": 4}""", """$['a\\b']""", IntegerType, 4)
+ testVariantGet("""{"a\tb": 5}""", """$['a\tb']""", IntegerType, 5)
+ testVariantGet("""{"a\nb": 6}""", """$['a\nb']""", IntegerType, 6)
+ testVariantGet("""{"a\bb": 11}""", """$['a\bb']""", IntegerType, 11)
+ testVariantGet("""{"a\fb": 12}""", """$['a\fb']""", IntegerType, 12)
+ testVariantGet("""{"a\rb": 13}""", """$['a\rb']""", IntegerType, 13)
+ testVariantGet("""{"a/b": 14}""", """$['a\/b']""", IntegerType, 14)
+ // Split the Unicode escape so Scala does not decode it.
+ testVariantGet("{\"a\\" + "u000bb\": 7}", "$['a\\" + "u000bb']", IntegerType, 7)
+ testVariantGet("""{"a\"b": 8}""", """$["a\"b"]""", IntegerType, 8)
+ testVariantGet("""{"a'b": 9}""", """$["a'b"]""", IntegerType, 9)
+ testVariantGet("""{"a\\xb": 15}""", """$['a\xb']""", IntegerType, 15)
+ testVariantGet("""{"a\\u12G4b": 17}""", "$['a\\" + "u12G4b']", IntegerType, 17)
+ testVariantGet("""{"a\\\"b": 18}""", """$['a\"b']""", IntegerType, 18)
+ testVariantGet("""{"a\\'b": 19}""", """$["a\'b"]""", IntegerType, 19)
+ testVariantGet(
+ "{\"a\\" + "uD83D\\" + "uDE00b\": 20}",
+ "$['a\\" + "uD83D\\" + "uDE00b']",
Review Comment:
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4303702861
##########
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):** `escapedKey` is a `def`, and `rep(escapedKey | index)` takes it by name, so every `parse()` call builds fresh key parsers and compiles this pattern (or its double-quoted twin) again once a bracket key is reached. The pattern also matches each character through a group alternation, which is much slower than the old `[^']*` character class. Constant paths are parsed once per expression and are unaffected. Non-foldable paths, however, are parsed for every row by `variant_get` / `try_variant_get` and by the dynamic-path branches of `variant_delete`, `variant_pick`, `variant_insert`, `variant_set` and `variant_array_append`, and the escape-decoding parser is the default. In a local `java.util.regex` measurement, compile plus match per parse went from about 0.27us to 0.81us for a 4-character key and from about 0.6us to 4.5us for a 200-character key. Dot-only paths are unchanged.
Consider compiling the dot and quoted-key regexes once as private vals (or making `escapedKey` a `val`). You could also use the equivalent unrolled form `[^'\\]*+(?:\\[\s\S][^'\\]*+)*+` and its `"` twin. It matched the same prefixes on the boundary cases I tried (an escaped quote before `]`, a trailing backslash, a doubled backslash, an empty key) and took about 1.7us compile plus match for the 200-character key. Neither change affects the accepted paths or the decode step.
**Verification:**
- **Compatibility:** All existing escape, legacy-mode, invalid-path, and failedAt round-trip expectations hold unchanged in both modes.
- **Regression:** Per-parse cost of a dynamic bracket-quoted path with a short key and with a long key is measured against the pre-change parser and shows no per-call regex compilation and no long-key slowdown beyond the decode step.
##########
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:
A follow-up works for me, since this only affects colon-syntax bracket keys that contain a backslash. Could you file a JIRA for validating and aliasing colon paths with VariantPathParser under the captured mode, including colon-syntax tests in both modes, and link it here? Ideally it lands before 4.4 ships, so the `:` syntax follows the same escape rules as the 4.4 migration note; until then `v:['a\'b']` is still rejected and escaped colon keys get undecoded column names.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4292171736","thread_id":"inline:4292171736","verdict_sha256":"1155a1c1929c77be417063e9cdb3e88306e8faaac151978d4b3789b6cfe8c389"} -->
##########
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:
Spark can't tell which grammar a connector applies to `path()`. In legacy mode, a pushed bracket key containing a backslash is right only for connectors that still read it literally, and wrong for connectors that follow the 4.4 escaped grammar that the `VariantExtraction` docs now describe. Declining keeps results correct, and the lost pushdown is limited to legacy mode with a path whose legacy and escaped parses actually differ. If that loss matters, the other option is to carry the mode on the connector `VariantGet` (it is `@Evolving`) and include it in `equals`/`hashCode`, so connectors that handle literal backslashes can keep accepting those predicates. Either way, today's translation hands connectors one path string that can name two different keys.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4292171745","thread_id":"inline:4292171745","verdict_sha256":"1155a1c1929c77be417063e9cdb3e88306e8faaac151978d4b3789b6cfe8c389"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4297965806
##########
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:
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4307640965
##########
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:
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4297942479
##########
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:
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4172988494
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -1640,6 +1666,22 @@ class VariantExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
checkInsertRecoverableError("""{"a.b": 5}""", """$['a.b'].c""", Literal(2),
"VARIANT_PATH_TYPE_MISMATCH",
Map("path" -> "$['a.b'].c", "failedAt" -> "$['a.b']", "functionName" -> "`variant_insert`"))
+ checkInsertRecoverableError("""{"a.'b": 5}""", """$["a.'b"].c""", Literal(2),
+ "VARIANT_PATH_TYPE_MISMATCH",
+ Map("path" -> "$[\"a.'b\"].c", "failedAt" -> "$[\"a.'b\"]",
+ "functionName" -> "`variant_insert`"))
+ checkInsertRecoverableError("""{"a.'\"b": 5}""", """$['a.\'"b'].c""", Literal(2),
+ "VARIANT_PATH_TYPE_MISMATCH",
+ Map(
+ "path" -> """$['a.\'"b'].c""",
+ "failedAt" -> """$["a.'\"b"]""",
+ "functionName" -> "`variant_insert`"))
+ checkInsertRecoverableError("""{"a.\\nb": 5}""", """$['a.\\nb'].c""", Literal(2),
Review Comment:
**Nit (P3):** `quoteKey` escapes control characters (`\b \f \n \r \t`, and `< 0x20` as `\uXXXX`), and `renderVariantPath` uses it to build round-trippable `failedAt`/`path` diagnostics. The parse-side decode of these escapes is now well covered (including `\b`/`\f`/`\r`), but the round-trip rendering cases here only exercise keys with a quote or a literal backslash -- none has an actual control character together with a `.`/`[` that forces the bracket-render branch. A regression that emitted a raw control char instead of its escape would stop the rendered path from re-parsing to the same segments, with no failing test. Worth adding one round-trip case whose key contains a control char.
**Verification:**
- **Regression:** If quoteKey emits a raw control char instead of its escape, the round-trip assertion fails.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4261645353
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -1640,6 +1666,22 @@ class VariantExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
checkInsertRecoverableError("""{"a.b": 5}""", """$['a.b'].c""", Literal(2),
"VARIANT_PATH_TYPE_MISMATCH",
Map("path" -> "$['a.b'].c", "failedAt" -> "$['a.b']", "functionName" -> "`variant_insert`"))
+ checkInsertRecoverableError("""{"a.'b": 5}""", """$["a.'b"].c""", Literal(2),
+ "VARIANT_PATH_TYPE_MISMATCH",
+ Map("path" -> "$[\"a.'b\"].c", "failedAt" -> "$[\"a.'b\"]",
+ "functionName" -> "`variant_insert`"))
+ checkInsertRecoverableError("""{"a.'\"b": 5}""", """$['a.\'"b'].c""", Literal(2),
+ "VARIANT_PATH_TYPE_MISMATCH",
+ Map(
+ "path" -> """$['a.\'"b'].c""",
+ "failedAt" -> """$["a.'\"b"]""",
+ "functionName" -> "`variant_insert`"))
+ checkInsertRecoverableError("""{"a.\\nb": 5}""", """$['a.\\nb'].c""", Literal(2),
Review Comment:
Added.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #59097: [SPARK-59218][SQL] Support escape sequences in Variant path parsing
URL: https://github.com/apache/spark/pull/59097
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4304375080
##########
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:
Created JIRA task ([link](https://issues.apache.org/jira/browse/SPARK-58540)).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4297929198
##########
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:
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4297867033
##########
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:
Would we be OK losing pushdown in legacy mode, even for connectors that already handle backslashes literally? Users enabling the flag to keep the old behavior could see a performance regression.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on PR #59097:
URL: https://github.com/apache/spark/pull/59097#issuecomment-6197342477
> @bojana-db can you fix merge conflicts?
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4307650454
##########
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:
I see, thanks. Pushdown is now declined in legacy mode only for paths whose legacy and default parses differ.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "bojana-db (via GitHub)" <gi...@apache.org>.
bojana-db commented on PR #59097:
URL: https://github.com/apache/spark/pull/59097#issuecomment-6266737784
> @bojana-db can you fix merge conflicts?
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59097:
URL: https://github.com/apache/spark/pull/59097#issuecomment-6260144046
@bojana-db can you fix merge conflicts?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59097:
URL: https://github.com/apache/spark/pull/59097#discussion_r4332529373
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -737,6 +737,41 @@ class VariantExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
// scalastyle:on nonascii
testVariantGet("[1, 2, 3]", "$[2147483647]", IntegerType, null)
+ testVariantGet("""{"a'b": 1}""", """$['a\'b']""", IntegerType, 1)
+ testVariantGet("""{"a']b": 1}""", """$['a\']b']""", IntegerType, 1)
+ testVariantGet("""{"a\nb":1,"a\\nb":2}""", """$.a\nb""", IntegerType, 2)
+ testVariantGet("""{"a\"b": 2}""", """$['a"b']""", IntegerType, 2)
+ testVariantGet("""{"a\\b": 4}""", """$['a\\b']""", IntegerType, 4)
+ testVariantGet("""{"a\tb": 5}""", """$['a\tb']""", IntegerType, 5)
+ testVariantGet("""{"a\nb": 6}""", """$['a\nb']""", IntegerType, 6)
+ testVariantGet("""{"a\bb": 11}""", """$['a\bb']""", IntegerType, 11)
+ testVariantGet("""{"a\fb": 12}""", """$['a\fb']""", IntegerType, 12)
+ testVariantGet("""{"a\rb": 13}""", """$['a\rb']""", IntegerType, 13)
+ testVariantGet("""{"a/b": 14}""", """$['a\/b']""", IntegerType, 14)
+ // Split the Unicode escape so Scala does not decode it.
+ testVariantGet("{\"a\\" + "u000bb\": 7}", "$['a\\" + "u000bb']", IntegerType, 7)
+ testVariantGet("""{"a\"b": 8}""", """$["a\"b"]""", IntegerType, 8)
+ testVariantGet("""{"a'b": 9}""", """$["a'b"]""", IntegerType, 9)
+ testVariantGet("""{"a\\xb": 15}""", """$['a\xb']""", IntegerType, 15)
+ testVariantGet("""{"a\\u12G4b": 17}""", "$['a\\" + "u12G4b']", IntegerType, 17)
+ testVariantGet("""{"a\\\"b": 18}""", """$['a\"b']""", IntegerType, 18)
+ testVariantGet("""{"a\\'b": 19}""", """$["a\'b"]""", IntegerType, 19)
+ testVariantGet(
+ "{\"a\\" + "uD83D\\" + "uDE00b\": 20}",
+ "$['a\\" + "uD83D\\" + "uDE00b']",
Review Comment:
**Nit (P3):** The pair branch of `escapeSeq` (`u[dD][89abAB][0-9a-fA-F]{2}\\u[dD][c-fC-F][0-9a-fA-F]{2}`) accepts either hex case and any high surrogate from D800 to DBFF, but the pair cases here use only upper-case escapes with a D8 high surrogate (`\uD83D\uDE00`). If the pattern were narrowed to upper-case hex, or to D8 high surrogates, every assertion would still pass. In the default mode, a path such as `$['\ud83d\ude00']` (the lower-case form that serializers such as Python's `json.dumps` emit) or `$['\uDBFF\uDFFF']` would then decode as two unpaired escapes kept as literal text, so `variant_get` and the manipulation functions would silently address a different key. One lower-case pair case and one DBxx pair case next to these tests would close the gap. Legacy mode decodes no escapes and is not affected.
**Verification:**
- **Regression:** Restricting the surrogate-pair pattern to upper-case hex digits makes the lower-case pair assertion fail.
- **Regression:** Restricting the high-surrogate range to D8xx makes the DB00-DBFF pair assertion fail.
- **Behavior:** With the current decoder, both new pair cases resolve the decoded supplementary-character key in default mode.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59218][SQL] Support escape sequences in Variant path parsing [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59097:
URL: https://github.com/apache/spark/pull/59097#issuecomment-6368414898
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/271386574a422140c55942a5a345ede858afb5cc
- merged into branch-4.x https://github.com/apache/spark/commit/e1782a369ddddd5dbe82aa02961d74fb12613a61
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org