Repository navigation
25352: fix: report regex compile failures as DataFusion errors, consistently across the regexp family - #363
martin-augment wants to merge 7 commits into
Conversation
`compile_regex` discarded the `regex::Error` and reported only the pattern, so a user could not see why a pattern or a flag was invalid. Report the diagnosis from the regex crate instead. It contains the pattern, so nothing is lost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`compile_regex` and `compile_and_cache_regex` move to a new `regex` module in datafusion-physical-expr-common, so that the physical expressions can use them too. They now return a `DataFusionError` instead of an `ArrowError`, and they take the name of the SQL function of the caller, so that an unsupported flag names the function that the user called instead of a fixed pair of names. `datafusion_functions::regex` re-exports both, so the paths that callers use still resolve. regexp_count and regexp_instr propagate the new error type. Their tests are updated, including three that asserted nothing because the expected message was parsed as part of the SQL statement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
regexp_match, regexp_like, regexp_replace and the `~` family of operators hand the pattern to an arrow kernel, which compiles it and reports a failure as an opaque `ArrowError::ComputeError`. A user saw an internal error instead of the reason the pattern was rejected. The kernel keeps compiling the pattern. Only when it fails does `explain_regexp_kernel_error` compile the patterns again, to report the first one that does not compile with the diagnosis of the regex crate. A query that succeeds compiles the pattern exactly as many times as before. The "global" flag check in regexp_match now tests every flags string that contains 'g', so "gi" no longer reaches the kernel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`simplify_regex_expr` compiles a literal pattern to rewrite it, and reported a pattern that does not compile as `Invalid regex`, wrapping the diagnosis in an `External` error. A literal pattern that does not compile is an error in the query text, known before execution, so report it as a plan error carrying the diagnosis of the regex_syntax crate. Every two argument regexp_like is simplified to the `~` operator, so this is the error that the most common spelling produces. Its wording now matches the one that the same pattern produces at execution time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`explain_regexp_kernel_error` collected the whole `patterns` array (and `flags`) into a `Vec<Option<&str>>` before looking for the pattern that did not compile. The collection is proportional to the length of the arrays, so a single invalid pattern in a large batch allocated and copied once per row on the error path. Borrow the arrays instead, through an accessor that holds the typed array and reads a row on demand. Explaining an error now allocates nothing beyond the pattern that `compile_regex` builds, whatever the length of the batch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9fJcUW5cbNf72vbWazRhh
`explain_regexp_kernel_error` is `pub` because `datafusion-physical-expr` and `datafusion-functions` call it from their own crates, not because it is meant for callers outside the workspace. Mark it `#[doc(hidden)]`, as the rest of the workspace marks the items that are public only to cross a crate boundary. `compile_regex` and `compile_and_cache_regex` keep their documentation: they were already public API before this branch moved them here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9fJcUW5cbNf72vbWazRhh
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Summary
WalkthroughThe change adds shared regex compilation, caching, flag validation, and kernel-error explanation utilities. Regex functions and binary kernels use DataFusion error types and shared diagnostics. Regex matching handles global flags consistently. Optimizer diagnostics and expected messages are updated. SQL logic tests add invalid-pattern, invalid-flag, NULL, and row-varying cases. Agent configuration files add review restrictions. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains from the reviewed regex diagnostic changes. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review
|
🤖 Augment PR SummarySummary: Standardizes regular-expression compilation failures as DataFusion errors. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| let rows = patterns | ||
| .len() | ||
| .max(flags.as_ref().map_or(0, StringValues::len)); | ||
| for row in 0..rows { |
There was a problem hiding this comment.
explain_regexp_kernel_error does not receive the values array, so this loop can compile a pattern from a NULL-valued row that Arrow skipped and report that different pattern when another row causes the kernel failure. This makes the new diagnostic incorrect for mixed-null batches.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
4 participants
25352: To review by AI