Feature/partitions #455
Merged
Feature/partitions #455
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kuba--
reviewed
Sep 4, 2018
cmd/gitbase/command/server.go
Outdated
| DisableSquash bool `long:"no-squash" description:"Disables the table squashing."` | ||
| TraceEnabled bool `long:"trace" env:"GITBASE_TRACE" description:"Enables jaeger tracing"` | ||
| ReadOnly bool `short:"r" long:"readonly" description:"Only allow read queries. This disables creating and deleting indexes as well." env:"GITBASE_READONLY"` | ||
| Parallelism uint `long:"parallelism" default:"4" description:"Maximum number of parallel threads per table."` |
| func NewDatabaseEngine( | ||
| readonly bool, | ||
| version string, | ||
| parallelism int, |
Contributor
Author
There was a problem hiding this comment.
Only needs to be uint in the flag, so the user can't input less than 0, after that it doesn't matter
| ab = ab.ReadOnly() | ||
| } | ||
|
|
||
| if parallelism > 1 { |
Contributor
There was a problem hiding this comment.
maybe 0 means by default, 1 - no parallelize
| if len(i.repos) == 0 || stringContains(i.repos, repo.ID) { | ||
| var err error | ||
| commits, err = NewCommitsByHashIter(repo, i.commitHashes) | ||
| var commitTreesHashIdx = CommitTreesSchema.IndexOf("commit_hash", CommitTreesTableName) |
Contributor
There was a problem hiding this comment.
why it's a package var? Can be a function?
Contributor
Author
There was a problem hiding this comment.
Because this value will be used in other functions
mcarmonaa
reviewed
Sep 5, 2018
Contributor
mcarmonaa
left a comment
mcarmonaa
left a comment
There was a problem hiding this comment.
I like how the new interfaces make simpler the table's implementation.
Apart from the questions, LGTM
| func (r *blobsTable) TransformUp(f sql.TransformNodeFunc) (sql.Node, error) { | ||
| return f(r) | ||
| func (r *blobsTable) WithFilters(filters []sql.Expression) sql.Table { | ||
| nt := *r |
Contributor
There was a problem hiding this comment.
I've seen this in the rest of the tables too, why can't r be used directly?
Contributor
Author
There was a problem hiding this comment.
Because it returns a new instance. By deref-ing the table, it makes a copy, so we don't have to manually create a table with all the fields, just modify the ones needed in the copy and return it as a pointer.
If in the future the table has more fields, we don't have to go to these methods and add the fields to the new instance.
commit_blobs.go
Outdated
| func (t *commitBlobsTable) TransformExpressionsUp(f sql.TransformExprFunc) (sql.Node, error) { | ||
| return t, nil | ||
| func (t *commitBlobsTable) WithProjection(colNames []string) sql.Table { | ||
| return t |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
Contributor
Author
There was a problem hiding this comment.
Nope, only blobs and files, which don't read the blob if the blob_content is not pushed down
commit_files.go
Outdated
| func (t *commitFilesTable) TransformExpressionsUp(f sql.TransformExprFunc) (sql.Node, error) { | ||
| return t, nil | ||
| func (t *commitFilesTable) WithProjection(colNames []string) sql.Table { | ||
| return t |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
commit_trees.go
Outdated
| func (t *commitTreesTable) TransformExpressionsUp(f sql.TransformExprFunc) (sql.Node, error) { | ||
| return t, nil | ||
| func (t *commitTreesTable) WithProjection(colNames []string) sql.Table { | ||
| return t |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
commits.go
Outdated
|
|
||
| return sql.NewSpanIter(span, repoIter), nil | ||
| func (r *commitsTable) WithProjection(colNames []string) sql.Table { | ||
| return r |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
ref_commits.go
Outdated
| func (t *refCommitsTable) TransformExpressionsUp(f sql.TransformExprFunc) (sql.Node, error) { | ||
| return t, nil | ||
| func (t *refCommitsTable) WithProjection(colNames []string) sql.Table { | ||
| return t |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
references.go
Outdated
| func (r *referencesTable) TransformExpressionsUp(f sql.TransformExprFunc) (sql.Node, error) { | ||
| return r, nil | ||
| func (r *referencesTable) WithProjection(colNames []string) sql.Table { | ||
| return r |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
Contributor
Author
There was a problem hiding this comment.
we could maybe just remove this methods so the tables don't implement ProjctedTable
remotes.go
Outdated
| func (r *remotesTable) TransformExpressionsUp(f sql.TransformExprFunc) (sql.Node, error) { | ||
| return r, nil | ||
| func (r *remotesTable) WithProjection(colNames []string) sql.Table { | ||
| return r |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
repositories.go
Outdated
|
|
||
| return sql.NewSpanIter(span, rowRepoIter), nil | ||
| func (r *repositoriesTable) WithProjection(colNames []string) sql.Table { | ||
| return r |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
tree_entries.go
Outdated
|
|
||
| return sql.NewSpanIter(span, repoIter), nil | ||
| func (r *treeEntriesTable) WithProjection(colNames []string) sql.Table { | ||
| return r |
Contributor
There was a problem hiding this comment.
doesn't accept this table pushdowns of projections?
mcarmonaa
approved these changes
Sep 5, 2018
Contributor
|
@erizocosmico I would say to remove all the table methods that are not being used (WithFilter, WithProjects and so on). The new API is modular and we don't need them if the table is not making use of them. |
Contributor
Author
|
Only |
Contributor
Author
|
Done @ajnavarro @mcarmonaa |
Contributor
Author
|
Changed the default parallelism @kuba-- |
Contributor
Author
|
Tests will pass once src-d/go-mysql-server#345 is merged and go-mysql-server upgraded. |
Contributor
Author
|
Tests should be passing now |
ajnavarro
suggested changes
Sep 6, 2018
Contributor
ajnavarro
left a comment
ajnavarro
left a comment
There was a problem hiding this comment.
There are some problems regarding the usage of partitions and join pushdown. Per example, this query:
SELECT count(*) FROM commits c NATURAL JOIN ref_commits r WHERE r.ref_name = 'HEAD';
It's using exchange nodes, but the pushdown node disappear compared to the previous version.
Contributor
Author
|
Just fixed the issue with the exchanges and the squashed tables |
jfontan
approved these changes
Sep 7, 2018
Contributor
|
More bugs found: Results are not the same: Result of |
Contributor
|
@erizocosmico can you rebase and update go-mysql-server with the new fixes please? |
Contributor
Author
|
yup |
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Contributor
Author
|
Rebased and upgraded |
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Contributor
Author
|
@ajnavarro I cannot reproduce the discrepancy in results with the query you gave me. Can you try again now to see if this was maybe fixed by the update of go-mysql-server? |
Contributor
|
@erizocosmico having the same problem. I'm using |
Contributor
Author
|
@ajnavarro with or without indexes? |
Contributor
|
@erizocosmico without indexes. |
Contributor
Author
|
I thought I had reproduced this but turns out the discrepancy was because I was running the old one on So, still can't reproduce this running it over all my Are you running both or the result of the old version is what you got when you ran it back then? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #450