Support "Select * from (a)" queries (when the table name is inside Parenthesis)

[eyalleshem]

Following the discussion in issue #154 ,
I tried to add support to parenthesis around table name,
waiting for reviews :)

[nickolay]

Thanks for the PR! As I suspected, this is rather complex - I didn't manage to recall all the necessary details in 15 minutes I had for this.

In order for me to be happy with the merge, the comments should be updated and it should be clear which syntax we wanted the parser to accept and which to reject: we have already established that Snowflake doesn't reject FROM ( table_name ), but FROM ( foo bar ) (which the patched version seems to accept) doesn't make sense to me. I'd also still like to hear from Nikhil and/or Andy.

[eyalleshem]

Hi ,
From (foo bar) should fail , i also added test for that .

I think added the bare table before the join - as I don't want the object to be "table with joins" but without join's - i think it's will be more correct that bare table will be just a table_factor (I also updated the comment's in that section) .

[eyalleshem]

It's look like the CI don't failed because code issue - but because some infrastructure issue..
Any option to re-run it ?

[nickolay]

It's look like the CI don't failed because code issue - but because some infrastructure issue..
Any option to re-run it ?

I don't know how travis permissions work, but it allows me to restart the build once I log in with github. Judging from https://rust-lang-nursery.github.io/rust-toolstate/ the nightly rustfmt we use is currently unavailable, so restarting the build until that's fixed will not help.

[nickolay]

I think added the bare table before the join - as I don't want the object to be "table with joins" but without join's - i think it's will be more correct that bare table will be just a table_factor

The joins in TableWithJoins are optional by design. A more appropriate name could be TableFactorOptionallyFollowedByJoins, but that's too long.

The code you have now strips the extraneous parens, while it would arguably be more correct to keep them in the AST via TableFactor::NestedJoin -- probably renamed it to Nested and its comments updated https://github.com/andygrove/sqlparser-rs/blob/172ba42001003bce79708141923ba6100cc7d4f2/src/ast/query.rs#L228-L232

I believe this would be less code, less nesting, and less intrusive patch overall.

[eyalleshem]

But - if i understand right you suggestion :

"From a" - will be parsed as :

TableWithJoins  {
    relation :  TableFactor::Table {
        name :a 
      [...] 
   }
}

while "From (a)" - will be parsed as :

TableWithJoins  {
    relation :  TableFactor::NestedJoin (
        TableWithJoins  {
              [Table factor for a]
         }
   ) 
}

even we don't have any nested join in the query...

[nickolay]

Right. TableFactor::NestedJoin was called that because standard SQL only allows joins to be nested. If we want other things to be nested, it should "probably renamed it to Nested and its comments updated," as I noted.

You may argue that an object representing the extra parentheses does NOT belong to an abstract syntax tree, but in other cases we prefer preserving detail in the AST, and in this particular case IMO this should make the particularly complex piece of the parser easier to manage.

[eyalleshem]

Changed according to your suggestion.
It's also will be better - as it's will support multi level of parenthesis.
(select *from ((a)))

[nickolay]

Thanks! This does seem simpler.

Please see the comments about the modified tests.

[eyalleshem]

Hi ,
The Ci failed because some clippy warning in era that i didn't touched (examples/cli.rs:43:26),
and i resolved all the comments - but for some reason the PR still in state of "Change requested" - don't sure how i need to solve that..

[nickolay]

The Ci failed because some clippy warning in era that i didn't touched (examples/cli.rs:43:26),

Thanks, I opened #159 to fix that.

some reason the PR still in state of "Change requested" - don't sure how i need to solve that..

I think you should be able to expand that section and "Re-request review" to change its state.

That's not the reason I didn't get to it, though, sorry!