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.

Sign up for GitHub

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

Jump to bottom

Fix long overflow for scalar function sequence #16867

Merged
merged 1 commit into from
Apr 19, 2023
Merged

Fix long overflow for scalar function sequence #16867

merged 1 commit into from
Apr 19, 2023

Conversation

chenjian2664
Copy link
Contributor

@chenjian2664 chenjian2664 commented Apr 4, 2023 •
edited
Loading

Description

Fix #16742

Additional context and related issues

Release notes

(x) Release notes are required, with the following suggested text:

# General
* Fix long overflow for scalar function sequence. ({issue}`16742`)

@cla-bot cla-bot bot added the cla-signed label Apr 4, 2023
@chenjian2664 chenjian2664 self-assigned this Apr 4, 2023
@chenjian2664
Copy link
Contributor Author

@kasiafi Could you guide me about how to add test cases for this, thanks!

@chenjian2664 chenjian2664 added bug Something isn't working correctness labels Apr 4, 2023
@chenjian2664 chenjian2664 marked this pull request as draft April 4, 2023 03:01
kasiafi
kasiafi reviewed Apr 4, 2023
View reviewed changes
Copy link
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

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

Looks good overall.

core/trino-main/src/main/java/io/trino/operator/scalar/SequenceFunction.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/operator/scalar/SequenceFunction.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/operator/scalar/SequenceFunction.java Outdated Show resolved Hide resolved
@chenjian2664 chenjian2664 marked this pull request as ready for review April 4, 2023 13:02
@chenjian2664 chenjian2664 requested a review from kasiafi April 4, 2023 23:58
kasiafi
kasiafi reviewed Apr 5, 2023
View reviewed changes
core/trino-main/src/main/java/io/trino/operator/scalar/SequenceFunction.java Outdated Show resolved Hide resolved
...trino-main/src/main/java/io/trino/operator/scalar/timestamp/SequenceIntervalDayToSecond.java
@@ -52,8 +51,7 @@ public static Block sequence(

checkValidStep(start, stop, step);

int length = toIntExact((stop - start) / step + 1L);
checkMaxEntry(length);
int length = checkMaxEntry((stop - start) / step + 1L);
Copy link
Member

Choose a reason for hiding this comment

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

It seems that you identified a couple of other overflow candidates. Would you mind filing an issue about potential overflows in SequenceIntervalDayToSecond and SequenceIntervalYearToMonth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, filed #16894
Should I link the issue as well, or remove the change in this pr and separate it.

@chenjian2664 chenjian2664 mentioned this pull request Apr 5, 2023
Closed
@chenjian2664 chenjian2664 requested a review from kasiafi April 13, 2023 03:34
@chenjian2664 chenjian2664 requested a review from kasiafi April 15, 2023 00:57
@chenjian2664
Copy link
Contributor Author

chenjian2664 commented Apr 17, 2023 •
edited
Loading

@kasiafi There are lots of possible cases that could overflow about the (stop - start) / step + 1L, start / step and stop / step, i.e.(start,stop,step) (Long.MIN_VALUE - 1, -1, 1) , (Long.MIN_VALUE, 0, 1), (Long.MIN_VALUE - 1, 0, 1) ...
So, I handle the special step (1 and -1) instead. After that we may only need to handle the stop - start case. Please help to review once you have time.

kasiafi
kasiafi reviewed Apr 18, 2023
View reviewed changes
Copy link
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

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

I tried to simplify the code basing on your previous approach. I extracted three different cases so that for each case it should be easy to prove that there can be no overflow.

We should have a test covering a big step with small start and stop values, e.g. (-5, 5, 1000), (-5, 5, 7).

core/trino-main/src/main/java/io/trino/operator/scalar/SequenceFunction.java Outdated Show resolved Hide resolved
kasiafi
kasiafi reviewed Apr 18, 2023
View reviewed changes
Copy link
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

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

I tried to simplify the code basing on your previous approach. I extracted three different cases so that for each case it should be easy to prove that there can be no overflow.

We should have a test covering a big step with small start and stop values, e.g. (-5, 5, 1000), (-5, 5, 7).

core/trino-main/src/main/java/io/trino/operator/scalar/SequenceFunction.java Outdated Show resolved Hide resolved
kasiafi
kasiafi approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

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

Good job!

@kasiafi kasiafi merged commit 1d49e8e into trinodb:master Apr 19, 2023
@github-actions github-actions bot added this to the 414 milestone Apr 19, 2023
@colebow colebow mentioned this pull request Apr 19, 2023
Closed
@chenjian2664 chenjian2664 deleted the fix_sequence branch April 20, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasiafi kasiafi kasiafi approved these changes

Assignees

@chenjian2664 chenjian2664

Labels
bug Something isn't working cla-signed correctness
Milestone
414
Development

Successfully merging this pull request may close these issues.

The scalar function sequence does not handle long overflow correctly
2 participants
@chenjian2664 @kasiafi