Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@agarciamontoro agarciamontoro commented Oct 11, 2019 •

edited

Loading

This PR fixes #834 with some changes:

Modify convertInt so it returns the smallest representation allowed. See acb2bc0.
~~(削除) Add a rule to convert integer literals to the specified types in the schema in INSERT nodes. See 2eb11bb. (削除ここまで)~~ After discussion with @erizocosmico (check Fix integer literals parsing #840 (comment) ), I've completely removed the integer conversion rule and moved its logic to Insert.Execute function. Much less code, which is always better. See instead 653fdde#diff-7e21a9fc108efea330a88d73d2a81023.
Adapt several parts of the code that broke after the above changes, namely:
- getInt64Literal function was modified so it converts any integer to int64. See 10d13fc.
- an explicit conversion to int64 was added in GROUP BY parsing, so any integer that fits is allowed. See 2320b07.
- the ROUND and YEARWEEK functions were also modified so they can work with all types of integers. See 97a2315
- most of the changes in this PR are in tests, as they were tailored to the previous int64-only design. These changes make the tests expect the right types. Review with care here, please 🙏 : 00a8833
- some more boilerplate changes in pilosa code so it handles 8 and 16 bits signed and unsigned integers values as well. See 5c38a0d
- I also needed to modify the conversion back to sqltypes.Value in numberT.SQL function, as it failed when 8 and 16 bits signed and unsigned integers values were passed, resulting in empty rows when doing, for example, SELECT 1. See ffae1d2

Maybe the most comfortable way to review this is:

Check first the new convertInt in acb2bc0 (if you know a way of making that code a bit more beautiful, I'd be glad to hear it)
Check then ~~(削除) the new rule for integer conversion 2eb11bb and its test 22600f9 (削除ここまで)~~ the modified Insert.Execute function: 653fdde#diff-7e21a9fc108efea330a88d73d2a81023
Review the minor modifications: 10d13fc, 2320b07, 97a2315, 5c38a0d and ffae1d2
Review the changes in existing tests, which will be tedious: 00a8833 and the new additions: 03f9766, 2917915, cc99d6c and fe3a84c.

This one is my first big contribution to go-mysql-server, so please review and test thoroughly. Comments and suggestions are more than welcome.

@agarciamontoro agarciamontoro requested a review from a team

October 11, 2019 10:27

erizocosmico

erizocosmico suggested changes

Oct 11, 2019

sql/analyzer/resolve_insert_literals.go Outdated Show resolved Hide resolved

sql/parse/parse.go Outdated Show resolved Hide resolved

sql/analyzer/resolve_insert_literals.go Outdated

var errWrongNumberOfValues = errors.NewKind("the number of values to insert differ from the expected columns")

func convertIntegerLiteralsInsert(ctx *sql.Context, analyzer *Analyzer, originalNode sql.Node) (sql.Node, error) {

Copy link

Contributor

@erizocosmico erizocosmico Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should let the table implementations handle this. When the literals are parsed, they are converted to the minimum type of int possible, then the table is in charge of converting it.

If we really want to make this transparent for table implementations, then I'd go for doing column.Type.Convert(theLiteralValue) inside the InsertInto execution method, but I would not add a new rule in the analyzer for this specific case.

WDYT @src-d/data-processing?

Copy link

Contributor Author

@agarciamontoro agarciamontoro Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the conversion should be done by us. If we convert string literals to proper dates, we should do the same here. I'd argue that int8 is as different from uint64 as a date is different from a string.

But maybe it makes sense to do it inside InsertInto, not as a new rule. I'll wait for the rest of opinions.

Copy link

Contributor Author

@agarciamontoro agarciamontoro Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved in 653fdde.

erizocosmico

erizocosmico reviewed

Oct 11, 2019

sql/parse/parse.go Outdated Show resolved Hide resolved

@agarciamontoro agarciamontoro force-pushed the fix.parsing branch 2 times, most recently from 37761f9 to 3034d2d Compare

October 11, 2019 10:55

Copy link

Contributor Author

agarciamontoro commented Oct 11, 2019

I'm just noticing that I did not run gofmt over the new files. I'll fix the formatting in the next commit.

@agarciamontoro agarciamontoro force-pushed the fix.parsing branch from ce3bd83 to 6d4af9b Compare

October 11, 2019 12:18

Copy link

Contributor Author

agarciamontoro commented Oct 11, 2019

As per @erizocosmico's comment (check #840 (comment)), I've completely removed the integer conversion rule and moved its logic to Insert.Execute function. Much less code, which is always better.

I've added it as a new commit (653fdde) for if someone still prefers the other alternative, but I can rebase it before merging.

@agarciamontoro agarciamontoro requested a review from erizocosmico

October 11, 2019 14:29

erizocosmico

erizocosmico approved these changes

Oct 11, 2019

Copy link

Contributor Author

agarciamontoro commented Oct 14, 2019

The latest four commits add missing cases to tests of modified functions. I think that CodeCoverage should be happy now.

@erizocosmico

Copy link

Contributor

erizocosmico commented Oct 14, 2019 •

edited

Loading

Can you rebase? there are some conflicts now

@erizocosmico erizocosmico requested a review from a team

October 14, 2019 09:17

agarciamontoro added 3 commits

October 14, 2019 11:18


 Modify convertInt so it returns the smallest representation allowed

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Add a rule to convert integer literals in INSERT

09e535d

- The integer literals in INSERT expressions are now converted to the type of
their corresponding column in the schema.
- sql.IsInteger has been fixed so that it recognizes smaller types.
Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Test the new rule to convert integers in INSERT nodes

25ce62f

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>

@agarciamontoro agarciamontoro force-pushed the fix.parsing branch from f468c84 to fe3a84c Compare

October 14, 2019 09:23

Copy link

Contributor Author

agarciamontoro commented Oct 14, 2019

Rebased. Codecov is still complaining, though. I'll take a look.

@erizocosmico

Copy link

Contributor

erizocosmico commented Oct 14, 2019

We can live with a -0.05%, don't worry 😄

ajnavarro

ajnavarro approved these changes

Copy link

Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

juanjux

juanjux approved these changes

carlosms

carlosms approved these changes

carlosms

carlosms reviewed

go.mod Outdated Show resolved Hide resolved

agarciamontoro added 7 commits

October 15, 2019 08:50


 Modify getInt64Literal to try to convert smaller representations

be23624

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Convert literals to specific int64 values in GROUP BY parsing

506c515

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Modify ROUND and YEARWEEK functions to manage all integer types

463c5e4

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Adapt all tests to new integer parsing

244439c

The tests now expect the correct types specified in the tables schema
Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Add missing cases to pilosa decodeGob and compare functions

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Add smaller integer types to the conversion to sqltypes.Value

95124bb

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Fix formatting of convertInt. HT @erizocosmico

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>

agarciamontoro added 6 commits

October 15, 2019 08:50


 Run gofmt over the whole codebase

daf82c5

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Move integer conversion from ad-hoc rule to Insert.Execute

f38b019

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Test missing cases in ROUND function

dcf2612

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Refactor type tests to cover all missing cases

2233b4d

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Add missing cases to pilosa decodeGob test

d436a96

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>


 Test parsing of integer literals

c0899fb

Signed-off-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>

@agarciamontoro agarciamontoro force-pushed the fix.parsing branch from fe3a84c to c0899fb Compare

October 15, 2019 06:51