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: [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]
