Fix integer literals parsing #840

agarciamontoro · 2019-10-11T10:27:34Z

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.

sql/analyzer/resolve_insert_literals.go

sql/parse/parse.go

erizocosmico · 2019-10-11T10:40:45Z

sql/analyzer/resolve_insert_literals.go

+
+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) {


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?

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.

Solved in 653fdde.

sql/parse/parse.go

agarciamontoro · 2019-10-11T11:00:02Z

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

agarciamontoro · 2019-10-11T14:29:10Z

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 · 2019-10-14T09:16:23Z

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

erizocosmico · 2019-10-14T09:17:30Z

Can you rebase? there are some conflicts now

Signed-off-by: Alejandro García Montoro <[email protected]>

- 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 <[email protected]>

Signed-off-by: Alejandro García Montoro <[email protected]>

agarciamontoro · 2019-10-14T09:29:24Z

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

erizocosmico · 2019-10-14T09:32:26Z

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

ajnavarro

Well done!

go.mod

Signed-off-by: Alejandro García Montoro <[email protected]>

The tests now expect the correct types specified in the tables schema Signed-off-by: Alejandro García Montoro <[email protected]>

Signed-off-by: Alejandro García Montoro <[email protected]>

agarciamontoro · 2019-10-15T06:52:09Z

Force pushed without changes in go.mod. Thanks for the catch, @carlosms!
I think it should be ready to merge now.

agarciamontoro requested a review from a team October 11, 2019 10:27

erizocosmico suggested changes Oct 11, 2019

View reviewed changes

erizocosmico reviewed Oct 11, 2019

View reviewed changes

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

agarciamontoro force-pushed the fix.parsing branch 2 times, most recently from 37761f9 to 3034d2d Compare October 11, 2019 10:55

agarciamontoro force-pushed the fix.parsing branch from ce3bd83 to 6d4af9b Compare October 11, 2019 12:18

agarciamontoro requested a review from erizocosmico October 11, 2019 14:29

erizocosmico approved these changes Oct 11, 2019

View reviewed changes

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

8754654

Signed-off-by: Alejandro García Montoro <[email protected]>

Test the new rule to convert integers in INSERT nodes

25ce62f

Signed-off-by: Alejandro García Montoro <[email protected]>

agarciamontoro force-pushed the fix.parsing branch from f468c84 to fe3a84c Compare October 14, 2019 09:23

ajnavarro approved these changes Oct 14, 2019

View reviewed changes

juanjux approved these changes Oct 14, 2019

View reviewed changes

carlosms approved these changes Oct 14, 2019

View reviewed changes

carlosms reviewed Oct 14, 2019

View reviewed changes

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 <[email protected]>

Convert literals to specific int64 values in GROUP BY parsing

506c515

Signed-off-by: Alejandro García Montoro <[email protected]>

Modify ROUND and YEARWEEK functions to manage all integer types

463c5e4

Signed-off-by: Alejandro García Montoro <[email protected]>

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 <[email protected]>

Add missing cases to pilosa decodeGob and compare functions

5616382

Signed-off-by: Alejandro García Montoro <[email protected]>

Add smaller integer types to the conversion to sqltypes.Value

95124bb

Signed-off-by: Alejandro García Montoro <[email protected]>

Fix formatting of convertInt. HT @erizocosmico

2395078

Signed-off-by: Alejandro García Montoro <[email protected]>

agarciamontoro added 6 commits October 15, 2019 08:50

Run gofmt over the whole codebase

daf82c5

Signed-off-by: Alejandro García Montoro <[email protected]>

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

f38b019

Signed-off-by: Alejandro García Montoro <[email protected]>

Test missing cases in ROUND function

dcf2612

Signed-off-by: Alejandro García Montoro <[email protected]>

Refactor type tests to cover all missing cases

2233b4d

Signed-off-by: Alejandro García Montoro <[email protected]>

Add missing cases to pilosa decodeGob test

d436a96

Signed-off-by: Alejandro García Montoro <[email protected]>

Test parsing of integer literals

c0899fb

Signed-off-by: Alejandro García Montoro <[email protected]>

agarciamontoro force-pushed the fix.parsing branch from fe3a84c to c0899fb Compare October 15, 2019 06:51

erizocosmico merged commit fb0e801 into src-d:master Oct 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix integer literals parsing #840

Fix integer literals parsing #840

agarciamontoro commented Oct 11, 2019 •

edited

Loading

erizocosmico Oct 11, 2019

agarciamontoro Oct 11, 2019

agarciamontoro Oct 11, 2019

agarciamontoro commented Oct 11, 2019

agarciamontoro commented Oct 11, 2019

agarciamontoro commented Oct 14, 2019

erizocosmico commented Oct 14, 2019 •

edited

Loading

agarciamontoro commented Oct 14, 2019

erizocosmico commented Oct 14, 2019

ajnavarro left a comment

agarciamontoro commented Oct 15, 2019 •

edited

Loading


		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) {

Fix integer literals parsing #840

Fix integer literals parsing #840

Conversation

agarciamontoro commented Oct 11, 2019 • edited Loading

erizocosmico Oct 11, 2019

Choose a reason for hiding this comment

agarciamontoro Oct 11, 2019

Choose a reason for hiding this comment

agarciamontoro Oct 11, 2019

Choose a reason for hiding this comment

agarciamontoro commented Oct 11, 2019

agarciamontoro commented Oct 11, 2019

agarciamontoro commented Oct 14, 2019

erizocosmico commented Oct 14, 2019 • edited Loading

agarciamontoro commented Oct 14, 2019

erizocosmico commented Oct 14, 2019

ajnavarro left a comment

Choose a reason for hiding this comment

agarciamontoro commented Oct 15, 2019 • edited Loading

agarciamontoro commented Oct 11, 2019 •

edited

Loading

erizocosmico commented Oct 14, 2019 •

edited

Loading

agarciamontoro commented Oct 15, 2019 •

edited

Loading