feat: revamp PRQL codegen framework #2669
Conversation
| pub fn write_ident_part(s: &str) -> String { | ||
| fn forbidden_start(c: char) -> bool { | ||
| !(c.is_ascii_lowercase() || matches!(c, '_' | '$')) | ||
| } | ||
| fn forbidden_subsequent(c: char) -> bool { | ||
| !(c.is_ascii_lowercase() || c.is_ascii_digit() || matches!(c, '_')) | ||
| } | ||
| let needs_escape = s.is_empty() | ||
| || s.starts_with(forbidden_start) | ||
| || (s.len() > 1 && s.chars().skip(1).any(forbidden_subsequent)); | ||
|
|
||
| if needs_escape { | ||
| format!("`{s}`") | ||
| } else { | ||
| s.to_string() | ||
| } | ||
| } |
There was a problem hiding this comment.
GPT4 wrote this as a regex:
| pub fn write_ident_part(s: &str) -> String { | |
| fn forbidden_start(c: char) -> bool { | |
| !(c.is_ascii_lowercase() || matches!(c, '_' | '$')) | |
| } | |
| fn forbidden_subsequent(c: char) -> bool { | |
| !(c.is_ascii_lowercase() || c.is_ascii_digit() || matches!(c, '_')) | |
| } | |
| let needs_escape = s.is_empty() | |
| || s.starts_with(forbidden_start) | |
| || (s.len() > 1 && s.chars().skip(1).any(forbidden_subsequent)); | |
| if needs_escape { | |
| format!("`{s}`") | |
| } else { | |
| s.to_string() | |
| } | |
| } | |
| use regex::Regex; | |
| use once_cell::sync::Lazy; | |
| static IDENT_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"^[_$a-z][_a-z\d]*$").unwrap()); | |
| pub fn write_ident_part(s: &str) -> String { | |
| if IDENT_REGEX.is_match(s) { | |
| s.to_string() | |
| } else { | |
| format!("`{}`", s) | |
| } | |
| } |
I actually though I did this before, but probably it was for something different...
There was a problem hiding this comment.
There was a problem hiding this comment.
I remembered where I did this before — is this the same as https://github.com/prql/prql/blob/aa3ed6196596e38b3dfffad63d0b590e9eeed535/prql-compiler/src/sql/gen_expr.rs#L783 ? (fine to have it twice unless it's definitely going to remain the same)
There was a problem hiding this comment.
This is the problem with GPT: it looks likes it's correct, but it's not. The regex does not quote empty strings, but it should.
In anycase, this snippet was copied from the gen_expr.rs a while back and was living on codegen branch for last few months.
If you feel like it needs to be improved, go ahead. But add tests if you are using GPT.
There was a problem hiding this comment.
Very good point!!
Yeah, agree it has that frailty. I have found it helpful for these sorts of "chore" tasks; agree tests become more important...
There was a problem hiding this comment.
Though in this case is it actually wrong? An empty string does get quoted by the regex.
There was a problem hiding this comment.
No, it's actually correct. An empty string does get quoted by the regex, which is correct. The logic has also been inverted.
But that's my point: it's more work validating the output as opposed to rewriting it. And things like this don't even need a rewrite or any more time spent on them.
There was a problem hiding this comment.
(I had already put #2678 in — which I think does use the version that gets the * correct, but not 100% sure)
|
Extremely impressive @aljazerzen ! |
From PRQL#2669 (comment) The behavior change is on handling `*`, which I _think_ is correct — at least it seems to compile OK.
Closes #1420