Use UAST columns from FilePairs table #206

smacker · 2018-03-06T12:17:04Z

Fixes #197
Fixes #222

Blocks #221

dpordomingo

LGTM

dpordomingo · 2018-03-06T14:27:57Z

server/repository/file_pairs.go

 	err := queryRow.Scan(&pair.ID,
 		&pair.Left.BlobID, &pair.Left.RepositoryID, &pair.Left.CommitHash,
-		&pair.Left.Path, &pair.Left.Content, &pair.Left.Hash,
+		&pair.Left.Path, &pair.Left.Content, &pair.Left.UAST, &pair.Left.Hash,


In some cases, UAST can be really big, and it is not needed for printing FilePairs, so it could be removed from the query.
Let's keep it in mind for the future in case we notice that getting the next file pair is delaying too much

dpordomingo

I'm leaving this red cross here to avoid merging this PR till we have considered/solved/scheduled migration

update: → #214

carlosms · 2018-03-09T14:35:45Z

cli/import/examples/example.sql

-		blob_id_a TEXT, repository_id_a TEXT, commit_hash_a TEXT, path_a TEXT, content_a TEXT,
-		blob_id_b TEXT, repository_id_b TEXT, commit_hash_b TEXT, path_b TEXT, content_b TEXT,
+		blob_id_a TEXT, repository_id_a TEXT, commit_hash_a TEXT, path_a TEXT, content_a TEXT, uast_a BLOB,
+		blob_id_b TEXT, repository_id_b TEXT, commit_hash_b TEXT, path_b TEXT, content_b TEXT, uast_b BLOB,


Type blob is not supported in postgres

Since this is an internal tool, and (internally) we're not using PostgreSQL but SQLite, I'd not block this PR because of this problem with PostgreSQL.
If Blobs cannot be easily solved for PostgreSQL, I'd merge as it is, and create a new issue to work on it when facing v1

@carlosms nice catch!

@carlosms @smacker Is there anything we could use instead that would allow to keep PostgreSQL compatibility?

We could do something similar to this:
https://github.com/src-d/code-annotation/blob/master/server/dbutil/dbutil.go#L38

And use bytea type for postgres. We'd have to check if it works different or has any other side effect.

If it requires extra work, I'd not block the merge as explained at #206 (review)

Nice! Thank you for pointing it out @carlosms

So, if that (and that's how it seems from example above) would require some custom code, to change the query at the runtime by replacing the type, right?

That's right. For the <INCREMENT_TYPE> the substitution is done in this method.

done by:

836d8db Add PostgreSQL support for BLOB

dpordomingo

Considering PostgreSQL issue pointed out by #206 (review) if we decide to merge without providing support to PostgreSQL, we should remove from our docs the mentions to the support of PostgreSQL.

On the other hand, if the PostgreSQL issue is solved, we can disregard this comment

dpordomingo · 2018-03-12T19:25:28Z

the plans for the migration were defined by #214

dpordomingo · 2018-03-20T18:06:07Z

I rebased and fixed a bug in the original branch with 04f34a0

@carlosms @bzz could you review this PR considering #206 (review) ?
If you don't strongly disagree with postponing the PostgreSQL support, we should merge in order to go to Prod with this new feature as soon as possible.

bzz · 2018-03-21T05:24:46Z

README.md


 The pieces of code to be labeled are called _file pairs_. They must be provided via an [SQLite](https://sqlite.org/) database. The database **must follow the expected schema**, please [follow this link](./cli/import/examples/example.sql) to see an example.

-The `import` command will use those file pairs to create a new [SQLite](https://sqlite.org/) or [PostgreSQL](https://www.postgresql.org/) database that will be used internally by the Annotation Tool. The destination database does not need to be empty, new imported file pairs can be added to previous imports.


There is a discussion on postgresql support but at any rate, even if this PR breaks it - removing existing Posgresql support in this PR does not seem like a good idea to me.

If merging fast of this work is required, I would propose create a new issue for adding the support of switching a type on top of this work, and then implementing it asap after this is merged, rather then having both - adding new feature and removing an old one in the same PR.

Let me explain the current situation:

I don't see the benefit of blocking the new feature (UASTs in FilePairs) till we fix a problem that we do not have yet (nobody is using PostgreSQL yet)

If we don't block this PR, it would be merged, so the PostgreSQL support would be broken the facto. If we leave the docs (and the code) as they currently are, it could confuse people trying to use PostgreSQL from now on.

I don't see the benefit of releasing something we already know it is broken.

I'm not purposing to drop definitively the PostgreSQL support; It can be resumed considering the issue Blob is not supported in PostgreSQL #222 . The commit dropping PostgreSQL support 94d41ab can be reverted; nothing is lost.

smacker · 2018-03-21T10:30:11Z

Thanks a lot for fixing bug with an order of columns. I must be missed it.

Though I would disagree with the last commit. Same thoughts as of @bzz above. If we need to merge this PR now, just merge it as it is with BLOB type column without removing code for postgres altogether. And later we can fix it using bytea type.

carlosms · 2018-03-21T13:44:49Z

I agree with @bzz and @smacker. The way I see it removing all the postgres code is too drastic. And if we decide to remove it, I'd prefer to see it in a new PR of its own.

dpordomingo

Could you @bzz @smacker @carlosms review the PR again considering the comments below?

I Rebased and restored the offending commit dropping PostgreSQL support as you suggested. I found some other problems with PostgreSQL that should be solved separately.

I rebased over master, and fixed some other problems that I found in the original branch:

7f05627 Avoid star select to avoid problems with some sources
edf2bbb Reorder columns to ensure that export command works

I also added support for BLOB data type if PostgreSQL DB is used. I tried it locally, and I could only import the first 100 FilePairs, that were properly processed and its content was the same that when they are imported into an SQLite database. The rest 1900 FilePairs could not be imported because of an unrelated problem as described by #224

836d8db Add PostgreSQL support for BLOB

bzz

LGTM

Thank you @dpordomingo for taking care, addressing all feedback. 👍 for having tests

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>

smacker requested a review from bzz March 6, 2018 12:17

dpordomingo approved these changes Mar 6, 2018

View reviewed changes

dpordomingo suggested changes Mar 6, 2018

View reviewed changes

carlosms suggested changes Mar 9, 2018

View reviewed changes

dpordomingo reviewed Mar 12, 2018

View reviewed changes

dpordomingo changed the title ~~Add UAST columns in DB~~ Use UAST columns from FilePairs table Mar 12, 2018

dpordomingo mentioned this pull request Mar 12, 2018

Add UAST to old FilePairs #214

Closed

12 tasks

dpordomingo force-pushed the add_uast_columns branch 2 times, most recently from dfbc202 to 8874abd Compare March 20, 2018 17:48

dpordomingo assigned dpordomingo and smacker Mar 20, 2018

dpordomingo added the feature-request New feature or request form the users label Mar 20, 2018

dpordomingo mentioned this pull request Mar 20, 2018

DB Migrations #221

Closed

bzz suggested changes Mar 21, 2018

View reviewed changes

dpordomingo mentioned this pull request Mar 21, 2018

Blob is not supported in PostgreSQL #222

Closed

dpordomingo force-pushed the add_uast_columns branch from 6e6169f to edf2bbb Compare March 22, 2018 17:08

dpordomingo requested review from bzz, carlosms and dpordomingo and removed request for dpordomingo March 22, 2018 17:22

dpordomingo unassigned smacker Mar 22, 2018

dpordomingo approved these changes Mar 22, 2018

View reviewed changes

carlosms approved these changes Mar 22, 2018

View reviewed changes

bzz reviewed Mar 23, 2018

View reviewed changes

bzz approved these changes Mar 23, 2018

View reviewed changes

Add UAST columns in DB

6ecf398

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>

dpordomingo added 5 commits March 23, 2018 17:04

Fix bug. FilePair queries has the 'uast' cols at the end

6a8491d

Comment exported vars to avoid lint issues

a63df24

Avoid star select to avoid problems with some sources

aeac5a6

Add PostgreSQL support for BLOB

704435a

Reorder columns to ensure that export command works

fa2f839

dpordomingo force-pushed the add_uast_columns branch from edf2bbb to fa2f839 Compare March 23, 2018 16:04

dpordomingo changed the base branch from master to release/v0.0.8 March 23, 2018 16:06

dpordomingo merged commit d644c52 into src-d:release/v0.0.8 Mar 23, 2018

This was referenced Apr 4, 2018

[WIP] release/v0.1 Adding UASTs + Going to Prod #237

Closed

release/v0.1 Adding UASTs + Going to Prod #238

Merged


		The pieces of code to be labeled are called _file pairs_. They must be provided via an [SQLite](https://sqlite.org/) database. The database must follow the expected schema, please [follow this link](./cli/import/examples/example.sql) to see an example.

		The `import` command will use those file pairs to create a new [SQLite](https://sqlite.org/) or [PostgreSQL](https://www.postgresql.org/) database that will be used internally by the Annotation Tool. The destination database does not need to be empty, new imported file pairs can be added to previous imports.

Use UAST columns from FilePairs table #206

Use UAST columns from FilePairs table #206

Uh oh!

Conversation

smacker commented Mar 6, 2018 • edited by dpordomingo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment • 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!

dpordomingo Mar 9, 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!

bzz Mar 14, 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!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

dpordomingo commented Mar 12, 2018

Uh oh!

dpordomingo commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bzz Mar 21, 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!

smacker commented Mar 21, 2018

Uh oh!

carlosms commented Mar 21, 2018

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

bzz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smacker commented Mar 6, 2018 •

edited by dpordomingo

Loading

dpordomingo left a comment •

edited

Loading

dpordomingo Mar 9, 2018 •

edited

Loading

bzz Mar 14, 2018 •

edited

Loading

dpordomingo commented Mar 20, 2018 •

edited

Loading

bzz Mar 21, 2018 •

edited

Loading