[Variant] Enahcne bracket access for VariantPath#9479
[Variant] Enahcne bracket access for VariantPath#9479klion26 wants to merge 1 commit intoapache:mainfrom
Conversation
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
Several comments, but only one that would block merge (error handling)
| ]); | ||
| assert_eq!(path, expected); | ||
|
|
||
| // field with number field(a number quoted with `'` will be treated as field, not index) |
There was a problem hiding this comment.
nit
| // field with number field(a number quoted with `'` will be treated as field, not index) | |
| // a number quoted with `'` is treated as field, not an index |
| .and_then(|s| s.strip_suffix('\'')) | ||
| { | ||
| // Quoted field name, e.g., ['field'] or ['123'] | ||
| VariantPathElement::field(inner.to_string()) |
There was a problem hiding this comment.
What are the string-escape rules here? Can the user embed a single quote as \'? as ''?
Or do they have to uri-encode it as %27? (do we even handle that?)
There was a problem hiding this comment.
Update: Here's the relevant grammar from JSONpath spec:
https://www.rfc-editor.org/rfc/rfc9535#name-selector
AI web search overview suggests the actual handling of delimiters and escapes tends to be vendor-specific (e.g. many database engines double up the delimiter as an escape)
There was a problem hiding this comment.
Ah! This function already handles \' escape syntax, L247 above.
| } else { | ||
| match unescaped.parse() { | ||
| Ok(idx) => VariantPathElement::index(idx), | ||
| Err(_) => VariantPathElement::field(unescaped), |
There was a problem hiding this comment.
This seems awkward? Especially in a fallible function? I don't think we should accept an unquoted string like [abc] or an invalid number like [123x]?
(I thought the jsonpath spec specifically forbade it, but I can't find the reference now)
| Ok(idx) => VariantPathElement::index(idx), | ||
| Err(_) => VariantPathElement::field(unescaped), | ||
| let element = if let Some(inner) = unescaped | ||
| .strip_prefix('\'') |
There was a problem hiding this comment.
JSONpath allows either ' or " as the string delimiter. Do we want to support both? Or take the intersection between SQL and JSONpath and only support single quotes?
2 participants
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
No