Update query parser to infer strings as ascii#277
Conversation
| parse_query_condition <- function(expr, debug=FALSE) { | ||
| .isComparisonOperator <- function(x) as.character(x) %in% c(">", ">=", "<", "<=", "==", "!=") | ||
| .isBooleanOperator <- function(x) as.character(x) %in% c("&&", "||", "!") | ||
| .isAscii <- function(x) grepl("^[a-zA-Z_]*$", x) |
There was a problem hiding this comment.
Do we want to keep the front anchor? Column names cannot start with digits...
Maybe "^[a-zA-Z_0-9]*" ?
There was a problem hiding this comment.
But this is checking column values, right? So we don't want to miss something like "1a".
There was a problem hiding this comment.
Gosh, you're right. I was so focussed on schema names.
In which case it is 'much worse' :) I.e. possibly payload is much more. Should we use [[:alpha:]] or a totally generic reg exp (meta) pattern.
There was a problem hiding this comment.
Sure, I like [[:alpha:]]. It should improve detection for non-a-z characters too! I'll update.
There was a problem hiding this comment.
Per ?regexp maybe [[:alnum:]] ?
eddelbuettel
left a comment
eddelbuettel
left a comment
There was a problem hiding this comment.
See the inlined comment -- I think we may at least want to keep the front-anchor.
aaronwolen
changed the title
eddelbuettel
left a comment
eddelbuettel
left a comment
There was a problem hiding this comment.
Thanks for the update based on what I riffed on over slack to you.
This had removed .isAscii which I just restored, if that doesn't pose a problem for you 🤠 I would be happy and delighted to merge
2 participants
Drops the anchors from
.isAscii's regex pattern to look for alpha characters anywhere in the value. Also adds a new test section where we can start checking for more type inference edge cases.