Change phrase/plain full text search syntax #1007
Conversation
|
I like the new syntax too, it looks clean. Because this is a breaking change it should go in postgrest 5. (The parameterized callProc pull request should as well because it changes the postgresql minimum version requirement.) Maybe we should get the other PRs merged first and make a 4.4 release with the new features, then follow it closely with a 5.0 release that contains these two breaking changes? |
|
I don't see the breaking change, the My idea was to avoid having a breaking change in v0.5 by having v0.4.4 with this PR merged and also after merging this to release v0.4.4 leaving my other PR's for v0.5 or V0.5-RC since they change core sql queries of PostgREST. |
|
Good point. I forgot the plain/phrase full text search selection is not yet part of an actual release. |
src/PostgREST/ApiRequest.hs
Outdated
| @@ -142,16 +142,8 @@ userApiRequest schema req reqBody | |||
| ActionInvoke{isReadOnly=True} -> partition (liftM2 (||) (isEmbedPath . fst) (hasOperator . snd)) flts | |||
| _ -> (flts, []) | |||
| flts = [ (toS k, toS $ fromJust v) | (k,v) <- qParams, isJust v, k /= "select", not (endingIn ["order", "limit", "offset", "and", "or"] k) ] | |||
| hasOperator val = foldr ((||) . flip T.isPrefixOf val) False ((<> ".") <$> (M.keys operators ++ ["not", show Plain, show Phrase])) | |||
| || hasLanguageFts val | |||
| hasOperator val = foldr ((||) . flip T.isPrefixOf val) False $ ((<> ".") <$> "not":M.keys operators) ++ ((<> "(") <$> M.keys ftsOperators) | |||
There was a problem hiding this comment.
This code is kind of gnarly, would the any function help simplify it?
There was a problem hiding this comment.
Like how about this:
hasOperator val =
any (`T.isPrefixOf` val)
$ map (`snoc` '.') ("not" : M.keys operators)
++ map (`snoc` '(') (M.keys ftsOperators)There was a problem hiding this comment.
The any function makes the function clearer, though the snoc/map instead of <>/<$> makes the function harder to read for me.
src/PostgREST/QueryBuilder.hs
Outdated
| Phrase -> "phraseto_tsquery(" | ||
| <> maybe "" (flip (<>) ", " . pgFmtLit) lang <> unknownLiteral val <> ") " | ||
| Fts op lang val -> | ||
| pgFmtFieldOp op <> "(" <> maybe "" (flip (<>) ", " . pgFmtLit) lang <> unknownLiteral val <> ") " |
There was a problem hiding this comment.
How about a little whitespace and a section rather than flip?
pgFmtFieldOp op <> "("
<> maybe "" ((<> ", ") . pgFmtLit) lang
<> unknownLiteral val <> ") "
|
A couple minor style suggestions, then I'd say it's good to merge! |
steve-chavez
force-pushed
the
change-fts-syntax
branch
from
3af5246 to
7ea1296
Compare
Changes the syntax for
phrase/plainintroduced in #964.Previous syntax:
New syntax:
New syntax is a bit shorter, it's more alike to the pg
{plain/phrase}to_tsqueryfunctions in that the first arg is the language(previous one has the language between plain/phrase and fts) and also it's more consistent because dots.are currently used like separators and the(language)fits better as an optional argument.Also have to mention that having
not.english..was kinda confusing insideand/oroperations, I think overall this new syntax is better and in addition it makes the code simpler.