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

exec: integer overflow isn't handled in plus or mult projection ops #36691

Closed
jordanlewis opened this issue Apr 9, 2019 · 2 comments
Closed
Assignees
Labels
A-sql-execution Relating to SQL execution. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Milestone

Comments

@jordanlewis
Copy link
Member

Currently, the addition projection ops don't handle integer overflow, but their equivalents in the Datum.Eval model do.

We should add overflow detection, assuming that it actually is important. It's unclear to me whether overflow detection is required by the SQL standard, or indeed important to users. Postgres does have it, but it was introduced relatively recently, in 2004: https://www.postgresql.org/message-id/[email protected]

The strategy recommended by Zukowski (http://oai.cwi.nl/oai/asset/14075/14075B.pdf) for integer overflow on values smaller than int64 is to perform the addition on the data type one larger than the input, OR the overflow bit into a result variable during the addition loop, and check the result of that bit after the loop.

For int64, we could use a similar structure to avoid conditional branches, but more checks will be required.

@jordanlewis jordanlewis added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Apr 9, 2019
@asubiotto asubiotto added this to the 19.2 milestone Apr 10, 2019
@asubiotto asubiotto added the A-sql-execution Relating to SQL execution. label Apr 10, 2019
@jordanlewis
Copy link
Member Author

@rafiss pretty sure you took care of this in entirety - please close if you agree.

@rafiss
Copy link
Collaborator

rafiss commented Jul 31, 2019

Yeah, all arithmetic overflow cases should be handled in vectorized now, see #38967

However, we don't really do projection correctly. See the snippet below. There's another example in this comment but I think we should have a separate issue for this case -- I just made #39189.

[email protected]:60323/defaultdb> create table foo (a int4 primary key);
CREATE TABLE

[email protected]:60323/defaultdb> insert into foo values(1);
INSERT 1

[email protected]:60323/defaultdb> select a+1 from foo;
   ?column?
+------------+
  2
(1 row)

[email protected]:60323/defaultdb> set experimental_vectorize=always;
SET

[email protected]:60323/?> select a+1 from foo;
pq: unable to vectorize execution plan: unable to columnarize render expression "@1 + 1:::INT8": BinaryExpr on int4 and int is unhandled

@rafiss rafiss closed this as completed Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-execution Relating to SQL execution. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

4 participants