jsonpath: interpret regex backspace escapes consistently - #174737
Open
Alignyx wants to merge 1 commit into
Open
Conversation
JSONPath like_regex passes its pattern to RE2, where backslash-b is a word-boundary constraint rather than PostgreSQL's backspace escape. This can match extra strings, reject an anchored backspace match, and fail to compile backspace escapes inside character classes. Translate unescaped backspace escapes at the JSONPath-specific regex cache key boundary, shared by parse-time validation and runtime compilation. Keep the stored pattern, literal q mode, escaped backslashes, and existing RE2 quoted spans unchanged. Do not change generic SQL regular expressions or attempt to implement the remaining PostgreSQL ARE differences. Add lexical boundary tests and SQL coverage for the reported queries, character classes, case folding, literal patterns, and SQL regex isolation. Resolves: cockroachdb#145270 Epic: none Release note (backward-incompatible change): JSONPath like_regex now treats an unescaped backslash-b as a backspace character instead of a word boundary, matching PostgreSQL's JSONPath behavior. Existing JSONPath expressions that relied on the previous word-boundary interpretation can return different results. This does not change ordinary SQL regular expression operators/functions, literal q patterns, or escaped backslashes.
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary and compatibility decision
This PR fixes the two reported
like_regexresults in #145270 by interpreting an unescaped JSONPath regex\bas backspace, as PostgreSQL does, instead of RE2's word-boundary assertion. The change is confined to JSONPath regex compilation; ordinary SQL regular expressions retain their existing behavior.This is intentionally backward-incompatible for existing JSONPath queries that relied on the old word-boundary interpretation. It is a narrow compatibility proposal, not a complete PostgreSQL ARE implementation or a claim that the issue's compatibility concerns are resolved. There is no opt-in, session setting, or version gate in this patch. The questions below ask whether this scoped behavior change is acceptable and how any remaining work should proceed.
Problem
The original queries are:
Both corrected queries return only the JSON string whose two characters are
aand U+0008 (backspace). They do not match the strings containing a literal backslash followed byb. Backspace escapes in character classes also fail regex validation on the baseline, although the corresponding explicit hex form works.Metamorphic relations and correct-result evidence
The target contract comes from PostgreSQL's character-entry escape definitions. For a nonliteral backspace atom, outside escaped-backslash literals and RE2 quoted spans, these spellings should select the same strings under that contract:
This supplies two transformations of the wrong query W: C_hex replaces the backspace atom with an explicit hex escape; C_unicode encodes the actual character at the JSONPath string layer. It is not an equivalence under CRDB's previous RE2 word-boundary contract, so passing this metamorphic check does not establish backward compatibility. Literal flag
q, escaped backslashes, and\Q...\Eregions are preservation controls, not interchangeable backspace spellings.The frozen investigation used this five-row matrix:
The C_unicode concatenation above constructs the same Unicode-escaped path text that was written literally in the archived investigation SQL. The same matrix was run with anchors:
"^a\\b$","^a\\x08$", and the Unicode spelling with the decoded U+0008 character betweenaand$. Three complete baseline runs agreed, and three complete candidate runs agreed:Additional held-out checks used
strpos, equality, andchr(8)rather than another regex to establish expected matches. Across 12 sample strings, five mismatch counts for unanchored, anchored, repeated-backspace, and quote-boundary cases changed from2/2/2/2/4to all zero. Literal, quoted, and explicit-hex control mismatch counts remained zero. This avoids relying solely on agreement between two uses of the same regex engine.Root cause and how it was localized
EXPLAIN (OPT, VERBOSE)for W and C_hex retained the same relational structure, differing in regex contents.EXPLAIN ANALYZEshowed the same five input rows being scanned, with the baseline W filter emitting three rows and C_hex emitting one. After the patch both emitted one. Exact selected values and IDs came from the separate SELECT results above, not from EXPLAIN ANALYZE.Additional row-engine and forced-DistSQL-setting controls reproduced the unanchored W result for each revision. The captured analyzed plans executed locally on n1: these are not cross-node or mixed-version execution traces. Source inspection supplies the compilation-path explanation; no instrumented branch trace is claimed.
The input JSON strings were already decoded correctly through
AsText(). The wrong transition occurs when the decoded regex pattern is compiled using Go regexp semantics:Changing JSON decoding would target the wrong layer. The JSONPath parser's
regexBinaryOpvalidates patterns usingReCache.GetRegexpWithFlags; runtimeevalRegexFuncuses the same cache/key interface. Both consultjsonpath.Regex.Pattern()when compilation is needed, making it the JSONPath-specific boundary shared by validation and execution.The relevant history informed this placement: 7cff4e8f introduced
like_regex; 26024c41 shared parse-time validation and runtime cache use; and c96c5a45 incorporated flags into the JSONPath cache key and compilation path. This patch preserves that shared boundary and flag separation.Repair and rationale
Regex.Pattern()now produces a temporary compilation string in which the applicable\btokens become\x08:qmode or when no\bsubstring is present.\Q...\Espans, including unclosed quoted tails. A quoted span ends at its first literal\E, consistent with the existing engine's handling even when another backslash precedes it.This is preferable to replacing the shared SQL regex engine or changing generic SQL regex keys because it limits the behavior change to JSONPath. It is preferable to a blind string replacement because literal backslashes and quoted patterns must not acquire backspace semantics. Putting the translation in the shared JSONPath key keeps parse-time validation and runtime matching consistent, including character classes that previously failed validation.
Existing regex compilation errors still propagate through the existing paths; parser errors retain the invalid-regex classification. Non-string JSON values are not coerced to strings by this patch. The changes do not add shared mutable state or alter cache locking.
Supported and preserved scenarios
Patterns in this table are regex-level spellings after JSONPath string decoding, unless a JSONPath regex flag is explicitly named.
\b, includinga\band^a\b$[\b]and tested range[\b-\n]2201Berrors. This is not a claim that every possible class combination was exhaustively tested.iqqto exercise flag isolation.\Q...\EquotingPattern()does not mutate formatted AST text.~controls and held-outregexp_replacecontrols retain their results.jsonb_path_query,jsonb_path_exists, andjsonb_path_matchare covered by native or investigation SQL using the shared implementation.Existing JSONPath regex flags
i,s,m, andqremain supported; the existing rejection of flagxis unchanged. The targeted new backspace/flag regressions coveriandq; passing the broader flag suites is not a claim of exhaustive new backspace tests for every flag combination.Not supported, not changed, or not yet established
\b: not preserved. Existing expressions can select different rows. There is no compatibility mode, opt-in, or version-gating mechanism in this commit.\B, remain unchanged. PostgreSQL's official definition of\Bis a backslash synonym, not the letter B; this patch does not translate it. Existing RE2 extensions remain available.Relation to the issue discussion and migration considerations
The issue explicitly raises existing users' reliance on CRDB behavior, discusses major-release changes with clear release notes, and favors documenting the engine difference. Restricting the patch to JSONPath reduces the affected surface but does not eliminate that objection. This PR is not evidence that the proposed exception has already been accepted.
For callers that intend to match an actual backspace, the explicit hex form shown in C_hex above works on both the baseline and candidate. This is also a usable workaround if the existing engine semantics are retained. Literal backslash-b matching continues to use literal
qmode or appropriately escaped backslashes.Callers that intend a word boundary must audit affected JSONPath expressions before adopting this change. Replacing
\bwith\x08is not a migration for that intent: it changes the meaning. This PR supplies no general word-boundary migration rewrite or legacy mode. Such a migration/compatibility design, associated user docs, and any release-note expansion remain follow-up work subject to the decision below.Completed local validation
Validation applies to commit
0fc8460c63ec9a9dcfb74fea881d420076613ec0, directly based on8812064a015d2faf99d3fc7e15880f94042954b0:TestRegexBackspacePatternunit test and eight native SQL regression queries cover the reported behavior and preservation boundaries above.local,local-vec-off,fakedist,fakedist-vec-off, andfakedist-disk.pkg/sql/sem/eval, and SQL package test targets passed; SQL ran in 16 shards.The full metamorphic/held-out investigation harness is not added to the product repository; the committed unit and native SQL regressions capture the selected behavior and preservation boundaries. These are completed local results, not a claim that upstream PR CI has passed or that all ARE compatibility cases have been tested.
Questions for maintainers
\bcompatibility change acceptable, despite the remaining backward-compatibility cost, or should jsonpath: incorrect handling oflike_regexwith escape characters in pattern #145270 instead retain RE2 behavior and be addressed through documentation and explicit-backspace workarounds?\bmismatch is accepted as resolved, should the broader documentation/compatibility work be tracked separately? The existing commit usesResolves: #145270for the reported mismatch; it does not claim that those broader concerns are implemented.No general ARE translator or compatibility mechanism is proposed as an already completed solution. The explicit-backspace workaround above is validated; the broader alternatives require a design decision and additional implementation/verification.
Resolves: #145270
Epic: none
Release note (backward-incompatible change): JSONPath like_regex now treats an unescaped backslash-b as a backspace character instead of a word boundary, matching PostgreSQL's JSONPath behavior. Existing JSONPath expressions that relied on the previous word-boundary interpretation can return different results. This does not change ordinary SQL regular expression operators/functions, literal q patterns, or escaped backslashes.