Relation parser fix

[cryogenian]

This automatically fixes slamdata/slamdata#2524 in master because the release with the fix should be patch-version.

@safareli Please take a look

[garyb]

I'd suggest we put the trys in here, rather than the parser definitions - in this case it doesn't matter too much, since they're only used in one place, but generally you want to only have try at the use site for things, as they make errors worse and can sometimes make it really difficult to fix things later due to inadvertent backtracking.

[garyb]

Actually, I'd suggest PC.try (tableRelation <|> variRelation <|> exprRelation) <|> parenRel kinda thing instead, since that should give slightly better errors and less unnecessary backtracking.

[safareli]

why:

PC.try (tableRelation <|> variRelation <|> exprRelation) <|> paren
?

first parser of variRelation is variableString which uses try.
first parser of tableRelation is ident which is defined using withToken which also uses try.
first parser of exprRelation is operator which is also defined using withToken.
I don't understand why we need try as it's already try-ed?

I haven't used try enough to relay tell what should we do here :/

Maybe a bit minimal test case will help to understand the problem?

[garyb]

Hmm, I checked out Maxim's branch and just experimented a bit - the new test cases seem to require the try around the first 3 cases in here, but from what it sounds like, they shouldn't really.

[garyb]

Ah I see, only exprRelation needs the try, since it is also paren-wrapped.

[garyb]

@safareli exprRelation needs the try because it will commit after operator "(" succeeds, even if the expr inside is already tryed.

[safareli]

Ah so

exprRelation ∷ ∀ m t. SqlParser m t (Sig.Relation t)
exprRelation = do
  operator "("
  e ← expr
  operator ")"
  ....

in exprRelation ( is parsed and commited, and expr is the part where it fails?
and after expr fails parenRelation is is alternatino but ( is consumed already so it fails too

parenRelation ∷ ∀ m t. SqlParser m t (Sig.Relation t)
parenRelation = do
  _ ← operator "("
  r ← relation
  _ ← operator ")"
  pure r

In that case wouldn't it be better in terms of error messages to have something like this?

impleRelation =
  tableRelation
  <|> variRelation
  <|> startingWIthParensRelation

startingWIthParensRelation = do
 _ ← operator "("
 exprRelation <|> parenRelation


parenWithoutStartParenRelation = do
  r ← relation
  _ ← operator ")"
  pure r

exprWithoutStartParenRelation = do
  e ← expr
  operator ")"
  ....

also if parser was represented with some free-ish construction during parsing actual parser could see that two parsers have common prefix and will avoid need for backtracking/try right?

[garyb]

I'm not sure it'd make a huge difference, I think the error message is going to be kinda bad either way as things are a bit ambiguous here - it's as much determined by the as that comes after the parens as it is by the contents of them.

[garyb]

Looks good, try thing aside!

[safareli]

can we name this expression, like others? "parenRelation" or something

[garyb]

Changing my review now 😉... let's just put the try on the usage of exprRelation.

[cryogenian]

@garyb Is this okay?
@safareli I named it parenRelation

[garyb]

👍

[zeeiyerWork]

@cryogenian - can we include this in the 427 branch !