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: add overloads for legitimate arithmetic operations #7414

Closed
knz opened this issue Jun 22, 2016 · 4 comments
Closed

sql: add overloads for legitimate arithmetic operations #7414

knz opened this issue Jun 22, 2016 · 4 comments

Comments

@knz
Copy link
Contributor

knz commented Jun 22, 2016

Found with #7409.

The following operations provide exact results and thus should be accepted:

  • decimal OP int / int OP decimal with OP in +, -, *, / should return decimal.
  • decimal IN <tuple> (example in sqllogictest/test/random/expr/slt_good_4.test:82808)
  • CASE <type X> WHEN <type Y>. SQL mandates that equality tests are always possible between heterogeneous types, so CASE/WHEN should allow this too (we can keep the type preference though).

Aggregates:

  • SUM on type <T> should return type <T> . The user can always cast SUM's argument to another type to perform the sum in that type instead.

Debatable:

  • float OP int and int OP float with OP in *, / could return float
  • decimal OP float and float OP decimal with OP in +, -, *, / could return decimal
@knz
Copy link
Contributor Author

knz commented Jun 22, 2016

cc @nvanbenschoten

@nvanbenschoten
Copy link
Member

@knz Thanks for putting this list together. Some of them I agree with, others I have reservations about:

The following operations provide exact results and thus should be accepted:

  • decimal OP int / int OP decimal with OP in +, -, *, / should return decimal.

Agreed!

  • decimal IN <tuple> (example in sqllogictest/test/random/expr/slt_good_4.test:82808)

I don't understand what you mean here.

  • CASE <type X> WHEN <type Y>. SQL mandates that equality tests are always possible between heterogeneous types, so CASE/WHEN should allow this too (we can keep the type preference though).

This is going to require some serious restructuring, as CASE statements currently use homogenous typing rules, and I'm not sure I see where it is needed. I'm also not sure what you mean by "equality tests are are always possible between heterogeneous types", as something like SELECT CASE 1 WHEN 's' should/will fail in Cockroach and in Postgres.

Aggregates:

  • SUM on type <T> should return type <T> . The user can always cast SUM's argument to another type to perform the sum in that type instead.

I don't think I agree. SUM on an INT column could easily overflow, which is why both Cockroach and Postgres aggregate in a larger data type. In fact, we currently mirror all (comparable) aggregate return types from https://www.postgresql.org/docs/9.5/static/functions-aggregate.html for the aggregates which we support.

Debatable:

  • float OP int and int OP float with OP in *, / could return float

But not addition and subtraction? Care to explain your reasoning?

  • decimal OP float and float OP decimal with OP in +, -, *, / could return decimal

I don't have strong feelings about this either.

@knz
Copy link
Contributor Author

knz commented Jun 23, 2016

  * |decimal IN <tuple>| (example in
    |sqllogictest/test/random/expr/slt_good_4.test:82808|)

I don't understand what you mean here.

The following query:

    SELECT - CASE WHEN NOT ( - 89 NOT IN ( - 85 + - - 70 + + ( + 50 ) /
+ 37 / 23, 32 / 95 * - COUNT ( * ) * + 86 + - 70, - 85, SUM ( DISTINCT -
85 ) - - CASE COUNT ( * ) WHEN ( 9 ) * + - ( + 2 ) THEN 22 / - 44 * + 32
/ - 86 WHEN - 70 THEN NULL ELSE - ( + NULLIF ( - 12, + 61 * 40 / 1 - 25
) ) END ) ) THEN NULL WHEN NULL BETWEEN NULL AND + NULLIF ( - 95, ( 80 )
) THEN + 84 ELSE NULL END AS col1;

Fails with:

pq: unsupported comparison operator: <decimal> NOT IN <tuple>
  * |CASE <type X> WHEN <type Y>|. SQL mandates that equality tests
    are always possible between heterogeneous types, so CASE/WHEN
    should allow this too (we can keep the type preference though).

This is going to require some serious restructuring, as CASE statements
currently use homogenous typing rules, and I'm not sure I see where it
is needed.

So the rule I have in mind is:

  • all WHEN clauses must be homogeneous (hard requirement)
  • all THEN/ELSE clauses must be homogeneous (hard requirement)
  • (WHEN and THEN types are independent, obviously)
  • the CASE clause (1st expression) is typed separately, should be
    homogeneous with WHEN, however if the CASE type does not match with
    WHEN, but both are numeric, this should be allowed.

I'm also not sure what you mean by "equality tests are are
always possible between heterogeneous types", as something like |SELECT
CASE 1 WHEN 's'| should/will fail in Cockroach and in Postgres.

Indeed. But SQL spec says, and PG supports, equality comparison between
arbitrary numeric types, doesn't it?

Aggregates:

  * |SUM on type <T>| should return type |<T>| . The user can always
    cast SUM's argument to another type to perform the sum in that
    type instead.

I don't think I agree. SUM on an INT column could easily overflow, which
is why both Cockroach and Postgres aggregate in a larger data type. In
fact, we currently mirror all (comparable) aggregate return types from
https://www.postgresql.org/docs/9.5/static/functions-aggregate.html for
the aggregates which we support.

I am willing to postpone a discussion on this until we look at the
issues above; because perhaps the test failures I am observing as due to
the argument type of the aggregate not inferred properly (or at all).

Debatable:

  * |float OP int| and |int OP float| with OP in |*, /| could return
    |float|

But not addition and subtraction? Care to explain your reasoning?

addition/substraction between int and float is "dangerous" because
people have this weird expectation that any number > 1 added or
substracted to an integer will make a new value, different from both
operands
. Obviously if the magnitude of the float is big enough this
property does not hold... Whereas it does hold for multiply and divide,
these will always change the value of the float.

I'm not very comfortable with this though so it's here merely a suggestion.

  * |decimal OP float| and |float OP decimal| with OP in |+, -, *,
    /| could return |decimal|

I don't have strong feelings about this either.

Me neither, but again I'd recommend delaying this conversation until we
make some progress with the first few items.

@knz
Copy link
Contributor Author

knz commented Sep 4, 2016

Most of the concerns were addressed by #7497 and #7756 and the other issues were sidelined by the closing of #7733.

@knz knz closed this as completed Sep 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants