Properly handle mixed implicit and explicit joins #109

benesch · 2019-06-08T03:50:26Z

Parse a query like

SELECT * FROM a NATURAL JOIN b, c NATURAL JOIN d

as the SQL specification requires, i.e.:

from: [
    TableReference {
        relation: TableFactor::Table("a"),
        joins: [Join {
            relation: TableFactor::Table("b"),
            join_operator: JoinOperator::Natural,
        }]
    },
    TableReference {
        relation: TableFactor::Table("c"),
        joins: [Join {
            relation: TableFactor::Table("d"),
            join_operator: JoinOperator::Natural,
        }]
    }
]

Previously we were parsing such queries as

relation: TableFactor::Table("a"),
joins: [
    Join {
        relation: TableFactor::Table("b"),
        join_operator: JoinOperator::Natural,
    },
    Join {
        relation: TableFactor::Table("c"),
        join_operator: JoinOperator::Implicit,
    },
    Join {
        relation: TableFactor::Table("d"),
        join_operator: JoinOperator::Natural,
    },
]

which did not make the join hierarchy clear.

coveralls · 2019-06-08T03:54:26Z

Pull Request Test Coverage Report for Build 330

163 of 174 (93.68%) changed or added relevant lines in 5 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.1%) to 91.892%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/sqlparser.rs	54	56	96.43%
tests/sqlparser_common.rs	88	97	90.72%

Files with Coverage Reduction	New Missed Lines	%
src/sqlparser.rs	1	92.76%
tests/sqlparser_common.rs	2	91.18%

Totals
Change from base Build 327:	-0.1%
Covered Lines:	4250
Relevant Lines:	4625

💛 - Coveralls

benesch · 2019-06-08T03:54:33Z

@nickolay I realize this is likely to be a bit prickly, as it is a substantial refactoring of a core AST. I wanted to flesh out what it looked like, though, and it didn't turn out to be too bad.

Note that because we now support nested joins, we could alternatively parse FROM a NATURAL JOIN b, c NATURAL JOIN d as:

relation: TableFactor::Table("a"),
joins: [
    Join { relation: TableFactor::Table("b"), operator: Natural },
    Join {
        relation: TableFactor::NestedJoin {
            base: TableFactor::Table("c"),
            joins: [Join { relation: TableFactor::("d"), operator: Natural }]
        },
        operator: Cross
    }
]

But perhaps downstream consumers will want to distinguish between implicit and explicit cross joins.

nickolay

Excellent, thank you! I had some ideas about naming / organization, but feel free to merge in any case.

nickolay · 2019-06-09T14:23:09Z

src/sqlast/query.rs

@@ -197,6 +192,23 @@ impl ToString for SQLSelectItem {
    }
 }

+/// A table reference is one table factor followed by any number of joins.


I get that this follows the names of the productions in ANSI SQL syntax, but maybe TableWithJoins?

Yeah, I find them hard to remember too. I've taken your suggestion for TableWithJoins.

nickolay · 2019-06-09T14:24:20Z

src/sqlast/query.rs

-        base: Box<TableFactor>,
-        joins: Vec<Join>,
-    },
+    NestedJoin(Box<TableReference>),


Given https://github.com/andygrove/sqlparser-rs/pull/108#pullrequestreview-247376543 maybe call it TableFactor::Nested?

Something about this whole design wasn't sitting right with me, so I decided to address the underlying issue you brought up in #108, rather than renaming this variant. I've pushed two commits that make it so that NestedJoin always contains a table reference with at least one join. A nice side effect of this work was supporting derived tables with set operations, like SELECT * FROM (((SELECT 1) UNION (SELECT 2)) t1 NATURAL JOIN t2).

nickolay · 2019-06-09T14:24:54Z

src/sqlparser.rs

            group_by,
            having,
        })
    }

+    pub fn parse_table_reference(&mut self) -> Result<TableReference, ParserError> {
+        let relation = self.parse_table_factor()?;
+        let joins = self.parse_joins()?;


Would parse_joins be useful elsewhere? Perhaps inline it here?

Nope, not that I can see. Inlined!

nickolay · 2019-06-09T22:12:24Z

Could you please extract the new parse_top_level() test (with the required changes in parse_statement) into its own commit and land it along with the already reviewed changes in this PR? Or land all of it and let me think about it later?

Making two parse attempts bothers me, plus the names from the spec (parse_derived_table_factor/parse_table_reference) are hard to remember; I'll need some time to let it sink in.

benesch · 2019-06-10T15:05:54Z

Could you please extract the new parse_top_level() test (with the required changes in parse_statement) into its own commit and land it along with the already reviewed changes in this PR?

Yep, working on that now. I'll open a new PR for the remaining changes as soon as this lands.

Parse a query like SELECT * FROM a NATURAL JOIN b, c NATURAL JOIN d as the SQL specification requires, i.e.: from: [ TableReference { relation: TableFactor::Table("a"), joins: [Join { relation: TableFactor::Table("b"), join_operator: JoinOperator::Natural, }] }, TableReference { relation: TableFactor::Table("c"), joins: [Join { relation: TableFactor::Table("d"), join_operator: JoinOperator::Natural, }] } ] Previously we were parsing such queries as relation: TableFactor::Table("a"), joins: [ Join { relation: TableFactor::Table("b"), join_operator: JoinOperator::Natural, }, Join { relation: TableFactor::Table("c"), join_operator: JoinOperator::Implicit, }, Join { relation: TableFactor::Table("d"), join_operator: JoinOperator::Natural, }, ] which did not make the join hierarchy clear.

This is a follow-up to apache#109 which moved the handling of Token::Comma from parse_table_and_joins to the `FROM` parser.

benesch requested a review from nickolay June 8, 2019 03:50

benesch mentioned this pull request Jun 8, 2019

Support nested joins #100

Merged

nickolay approved these changes Jun 9, 2019

View reviewed changes

benesch force-pushed the implicit-join-fix branch 2 times, most recently from 7210715 to 94c23f3 Compare June 10, 2019 15:05

benesch added 2 commits June 10, 2019 11:11

Support VALUES and parenthesized SELECTs as top-level statements

ce171c2

benesch force-pushed the implicit-join-fix branch from 94c23f3 to ce171c2 Compare June 10, 2019 15:11

benesch merged commit 5896f01 into apache:master Jun 10, 2019

benesch deleted the implicit-join-fix branch June 10, 2019 15:17

benesch mentioned this pull request Jun 10, 2019

Refine join parsing #111

Merged

nickolay added a commit to nickolay/sqlparser-rs that referenced this pull request Jun 13, 2019

Add a testcase, which passes thanks to PR apache#109

32cf36e

nickolay added a commit to nickolay/sqlparser-rs that referenced this pull request Jun 13, 2019

Reduce nesting in parse_table_and_joins

7041850

This is a follow-up to apache#109 which moved the handling of Token::Comma from parse_table_and_joins to the `FROM` parser.

nickolay mentioned this pull request Apr 18, 2022

join syntax #459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Properly handle mixed implicit and explicit joins #109

Properly handle mixed implicit and explicit joins #109

Uh oh!

benesch commented Jun 8, 2019

Uh oh!

coveralls commented Jun 8, 2019 •

edited

Loading

Uh oh!

benesch commented Jun 8, 2019

Uh oh!

nickolay left a comment

Uh oh!

nickolay Jun 9, 2019

Uh oh!

benesch Jun 10, 2019

Uh oh!

nickolay Jun 9, 2019

Uh oh!

benesch Jun 9, 2019

Uh oh!

nickolay Jun 9, 2019

Uh oh!

benesch Jun 10, 2019

Uh oh!

nickolay commented Jun 9, 2019

Uh oh!

benesch commented Jun 10, 2019

Uh oh!

Uh oh!

Properly handle mixed implicit and explicit joins #109

Properly handle mixed implicit and explicit joins #109

Uh oh!

Conversation

benesch commented Jun 8, 2019

Uh oh!

coveralls commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 330

💛 - Coveralls

Uh oh!

benesch commented Jun 8, 2019

Uh oh!

nickolay left a comment

Choose a reason for hiding this comment

Uh oh!

nickolay Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

benesch Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

nickolay Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

benesch Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

nickolay Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

benesch Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

nickolay commented Jun 9, 2019

Uh oh!

benesch commented Jun 10, 2019

Uh oh!

Uh oh!

coveralls commented Jun 8, 2019 •

edited

Loading