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