internal/rule: fix squashjoins rule to squash projections properly too #338

mcarmonaa · 2018-06-26T10:38:48Z

Fixes #322

The bug came because the projects nodes over inner joins weren't squashed, so they stayed chained and pointing to wrong field indexes after the table squashing

GroupBy
 ├─ Aggregate(COUNT(1), refs.repository_id)
 ├─ Grouping(refs.repository_id)
 └─ Project(refs.repository_id, refs.commit_hash, refs.ref_name, commits.commit_author_name, commits.commit_author_email, commits.commit_author_when, commits.committer_name, commits.committer_email, commits.committer_when, commits.commit_message, commits.tree_hash, commits.commit_parents, commit_blobs.blob_hash)
     └─ Project(refs.repository_id, refs.commit_hash, refs.ref_name, commits.commit_author_name, commits.commit_author_email, commits.commit_author_when, commits.committer_name, commits.committer_email, commits.committer_when, commits.commit_message, commits.tree_hash, commits.commit_parents)
         └─ Filter(refs.repository_id = commit_blobs.repository_id AND refs.commit_hash = commit_blobs.commit_hash)
             └─ SquashedTable(refs, commits, commit_blobs)
                 ├─ Columns
                 │   ├─ Column(repository_id, TEXT, nullable=false)
                 │   ├─ Column(ref_name, TEXT, nullable=false)
                 │   ├─ Column(commit_hash, TEXT, nullable=false)
                 │   ├─ Column(repository_id, TEXT, nullable=false)
                 │   ├─ Column(commit_hash, TEXT, nullable=false)
                 │   ├─ Column(commit_author_name, TEXT, nullable=false)
                 │   ├─ Column(commit_author_email, TEXT, nullable=false)
                 │   ├─ Column(commit_author_when, TIMESTAMP, nullable=false)
                 │   ├─ Column(committer_name, TEXT, nullable=false)
                 │   ├─ Column(committer_email, TEXT, nullable=false)
                 │   ├─ Column(committer_when, TIMESTAMP, nullable=false)
                 │   ├─ Column(commit_message, TEXT, nullable=false)
                 │   ├─ Column(tree_hash, TEXT, nullable=false)
                 │   ├─ Column(commit_parents, JSON, nullable=false)
                 │   ├─ Column(repository_id, TEXT, nullable=false)
                 │   ├─ Column(commit_hash, TEXT, nullable=false)
                 │   └─ Column(blob_hash, TEXT, nullable=false)
                 └─ Filters
                     ├─ refs.repository_id = commit_blobs.repository_id
                     ├─ refs.commit_hash = commit_blobs.commit_hash
                     ├─ refs.repository_id = commits.repository_id
                     ├─ refs.commit_hash = commits.commit_hash
                     └─ refs.ref_name = "HEAD"

Now the project nodes are squashed too, so it works properly:

GroupBy
 ├─ Aggregate(COUNT(1), refs.repository_id)
 ├─ Grouping(refs.repository_id)
 └─ Project(refs.repository_id, refs.commit_hash, refs.ref_name, commits.commit_author_name, commits.commit_author_email, commits.commit_author_when, commits.committer_name, commits.committer_email, commits.committer_when, commits.commit_message, commits.tree_hash, commits.commit_parents, commit_blobs.blob_hash)
     └─ Filter(refs.repository_id = commit_blobs.repository_id AND refs.commit_hash = commit_blobs.commit_hash)
         └─ SquashedTable(refs, commits, commit_blobs)
             ├─ Columns
             │   ├─ Column(repository_id, TEXT, nullable=false)
             │   ├─ Column(ref_name, TEXT, nullable=false)
             │   ├─ Column(commit_hash, TEXT, nullable=false)
             │   ├─ Column(repository_id, TEXT, nullable=false)
             │   ├─ Column(commit_hash, TEXT, nullable=false)
             │   ├─ Column(commit_author_name, TEXT, nullable=false)
             │   ├─ Column(commit_author_email, TEXT, nullable=false)
             │   ├─ Column(commit_author_when, TIMESTAMP, nullable=false)
             │   ├─ Column(committer_name, TEXT, nullable=false)
             │   ├─ Column(committer_email, TEXT, nullable=false)
             │   ├─ Column(committer_when, TIMESTAMP, nullable=false)
             │   ├─ Column(commit_message, TEXT, nullable=false)
             │   ├─ Column(tree_hash, TEXT, nullable=false)
             │   ├─ Column(commit_parents, JSON, nullable=false)
             │   ├─ Column(repository_id, TEXT, nullable=false)
             │   ├─ Column(commit_hash, TEXT, nullable=false)
             │   └─ Column(blob_hash, TEXT, nullable=false)
             └─ Filters
                 ├─ refs.repository_id = commit_blobs.repository_id
                 ├─ refs.commit_hash = commit_blobs.commit_hash
                 ├─ refs.repository_id = commits.repository_id
                 ├─ refs.commit_hash = commits.commit_hash
                 └─ refs.ref_name = "HEAD"

Signed-off-by: Manuel Carmona manu.carmona90@gmail.com

ajnavarro · 2018-06-26T15:09:15Z

internal/rule/squashjoins.go

+func squashProjects(parent, child *plan.Project) (sql.Node, error) {
+	projections := []sql.Expression{}
+	for _, expr := range parent.Expressions() {
+		parentField, ok := expr.(*expression.GetField)


If an expression is a literal, will it fail? Example:

SELECT count(1),1 , refs.repository_id FROM refs NATURAL JOIN commits NATURAL JOIN commit_blobs WHERE refs.ref_name = 'HEAD' GROUP BY refs.repository_id

That query doesn't work because of how the GroupBy is implemented right now. It only allows expressions in the select which are aggregations or are in the group by too, not because of the changes in this PR.

The following query, for example. does work:

MySQL [(none)]> SELECT 1,refs.repository_id FROM refs NATURAL JOIN commits NATURAL JOIN commit_blobs WHERE refs.ref_name = 'HEAD' limit 10; +------+---------------+ | 1 | repository_id | +------+---------------+ | 1 | enry | | 1 | enry | | 1 | enry | | 1 | enry | | 1 | enry | | 1 | enry | | 1 | enry | | 1 | enry | | 1 | enry | | 1 | enry | +------+---------------+ 10 rows in set (0.02 sec)

then, how can we reach that error?

Sorry @ajnavarro, I think I'm not understanding you. Do you mean that if a literal appears in the projection the query should fail? Anyway that's something not related to this PR, I guess.

Should I open an issue for it?

I'm trying to understand when this error will be thrown.

As I can see on the squashProjects you are getting the expressions on the projection and checking if the type is a Getfield expression. If not, you are throwing an error.

There are some cases when a projection can contain other expressions than GetField (per example, a Function or a Literal), and that is not necessarily an error.

🤓 Now I see what you mean. The thing is that the projections to squash coming from the replacement of natural joins by inner joins, and those projections only contains GetFields. That's the reason because the chained projections appear, so functions and literals written in a query aren't going to be squashed.

mcarmonaa · 2018-06-28T09:35:32Z

@ajnavarro I already committed the change to not throw an error

ajnavarro · 2018-06-28T09:50:27Z

internal/rule/squashjoins.go

+		}
+
+		squashedProject, err := squashProjects(project, child)
+		if err != nil {


Other error than errWrongProjection can happen.

Actually, cannot, but this code is a little bit misleading. Maybe instead of return squashedProject and err we should return squasedProject and ok

ajnavarro · 2018-06-28T10:26:42Z

@mcarmonaa can you rebase and merge please?

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa requested a review from a team June 26, 2018 10:38

kuba-- approved these changes Jun 26, 2018

View reviewed changes

erizocosmico approved these changes Jun 26, 2018

View reviewed changes

ajnavarro reviewed Jun 26, 2018

View reviewed changes

erizocosmico mentioned this pull request Jun 28, 2018

Proposal: Make squash join rule active by default #340

Closed

erizocosmico mentioned this pull request Jun 28, 2018

cmd/gitbase: make squash rule active by default #345

Merged

ajnavarro suggested changes Jun 28, 2018

View reviewed changes

mcarmonaa force-pushed the fix/squash-natural-joins branch from 2917d49 to ceb33f3 Compare June 28, 2018 10:00

erizocosmico approved these changes Jun 28, 2018

View reviewed changes

ajnavarro approved these changes Jun 28, 2018

View reviewed changes

mcarmonaa added 2 commits June 28, 2018 12:28

internal/rule: fix squashjoins rule to squash projections properly too

8d7298a

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

internal/rule: don't throw error on squashing projects

015bbfa

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa force-pushed the fix/squash-natural-joins branch from ceb33f3 to 015bbfa Compare June 28, 2018 10:28

mcarmonaa merged commit f69fe09 into src-d:master Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

internal/rule: fix squashjoins rule to squash projections properly too #338

internal/rule: fix squashjoins rule to squash projections properly too #338

Uh oh!

mcarmonaa commented Jun 26, 2018 •

edited

Loading

Uh oh!

ajnavarro Jun 26, 2018 •

edited

Loading

Uh oh!

mcarmonaa Jun 26, 2018

Uh oh!

ajnavarro Jun 27, 2018

Uh oh!

mcarmonaa Jun 27, 2018

Uh oh!

ajnavarro Jun 27, 2018

Uh oh!

mcarmonaa Jun 27, 2018

Uh oh!

mcarmonaa commented Jun 28, 2018

Uh oh!

ajnavarro Jun 28, 2018

Uh oh!

ajnavarro Jun 28, 2018

Uh oh!

mcarmonaa Jun 28, 2018

Uh oh!

ajnavarro commented Jun 28, 2018

Uh oh!

Uh oh!

internal/rule: fix squashjoins rule to squash projections properly too #338

internal/rule: fix squashjoins rule to squash projections properly too #338

Uh oh!

Conversation

mcarmonaa commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajnavarro Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarmonaa commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajnavarro commented Jun 28, 2018

Uh oh!

Uh oh!

mcarmonaa commented Jun 26, 2018 •

edited

Loading

ajnavarro Jun 26, 2018 •

edited

Loading