Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: integer division returns float, sqllogic test expects int #3271

Closed
maddyblue opened this issue Dec 1, 2015 · 7 comments
Closed

sql: integer division returns float, sqllogic test expects int #3271

maddyblue opened this issue Dec 1, 2015 · 7 comments
Labels
A-sql-semantics C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone

Comments

@maddyblue
Copy link
Contributor

The following (currently disabled) test performs integer division. Cockroach returns a float instead of the expected int. Integer division is the only binary operator that takes integers and returns a float.

https://github.com/cockroachdb/sqllogictest/blob/a88396b84bb1fe62edf2e072e585d68c0b2ecdca/test/select1.test#L101

I propose allowing the test logic to accept both ints and floats when it expects an int. This would allow any sqllogic test using integer division to succeed. It would be an additional if statement at

case 'I':
.

@maddyblue
Copy link
Contributor Author

Based on how postgres works, I think we should change integer division to return an integer:

postgres=# create table t (a INT, b INT, c FLOAT, d FLOAT);
CREATE TABLE
postgres=# insert into t values (2, 3, 4, 5);
INSERT 0 1
postgres=# select a/b, b/c, c/d from t;
 ?column? | ?column? | ?column? 
----------+----------+----------
        0 |     0.75 |      0.8
(1 row)

@petermattis petermattis added SQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. and removed SQL labels Feb 12, 2016
@petermattis petermattis modified the milestone: Beta Feb 14, 2016
@knz
Copy link
Contributor

knz commented Feb 17, 2016

Since #3543 fixes the original problem, and the proposed new typing system would type division coherently with its operands, is this issue still relevant?

@tamird
Copy link
Contributor

tamird commented Feb 17, 2016

#3543 was closed. This issue is still relevant.

@petermattis petermattis modified the milestones: 1.0, Beta Feb 21, 2016
@knz
Copy link
Contributor

knz commented Jun 6, 2016

See also #6642.

@nvanbenschoten
Copy link
Member

I don't see MySQL's handling of int division mentioned here yet. MySQL has a similar issue where INT/INT -> REAL. However, they also provide a floor division operator DIV, similar to our //. To get around this, there are a number of skipif mysql and onlyif mysql directives in the test files which use DIV in place of / when MySQL requires it.

Following this, we could write a script to generate appropriate alternatives for our FLOORDIV operator. This seems like a clean way to handle this issue.

@maddyblue
Copy link
Contributor Author

@knz is this fixed with the -flex-types option?

@knz
Copy link
Contributor

knz commented Jul 24, 2016

Yes it is alleviated by -flex-types and what isn't will be addressed by #7733. Let's close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-semantics C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants