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

opt: push limit into offset #38660

Merged
merged 2 commits into from
Jul 9, 2019

Conversation

ridwanmsharif
Copy link

@ridwanmsharif ridwanmsharif commented Jul 3, 2019

This change pushes the limit into an offset whenever possible.
This shouldn't worsen any plan but does allow the GetLimitedScans
rule to fire in more scenarios.

Fixes #30416.
This is currently blocked on #38659.

Release note: None

@ridwanmsharif ridwanmsharif requested a review from a team as a code owner July 3, 2019 17:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @ridwanmsharif)


pkg/sql/opt/norm/custom_funcs.go, line 1695 at r1 (raw file):

	firstVal := int64(*first.(*tree.DInt))
	secondVal := int64(*second.(*tree.DInt))
	return tree.NewDInt(tree.DInt(firstVal + secondVal))

We should check for overflow here (or somewhere in the rule).

Copy link
Author

@ridwanmsharif ridwanmsharif left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/opt/norm/custom_funcs.go, line 1695 at r1 (raw file):

Previously, RaduBerinde wrote…

We should check for overflow here (or somewhere in the rule).

Done.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @ridwanmsharif)


pkg/sql/opt/norm/custom_funcs.go, line 1694 at r3 (raw file):

// AddConsts adds the numeric constants together. AddConsts assumes the sum
// will not overflow. Call DoesSumOverflow on the constants to guarantee this.
func (c *CustomFuncs) AddConsts(first tree.Datum, second tree.Datum) tree.Datum {

[nit] rename to AddConstInts


pkg/sql/opt/norm/custom_funcs.go, line 1697 at r3 (raw file):

	firstVal := int64(*first.(*tree.DInt))
	secondVal := int64(*second.(*tree.DInt))
	sum, _ := arith.AddWithOverflow(firstVal, secondVal)

check ok and panic(errors.AssertionFailedf(..)) if it's false


pkg/sql/opt/norm/custom_funcs.go, line 1702 at r3 (raw file):

// DoesSumOverflow returns true if the addition of the two integers overflows.
func (c *CustomFuncs) DoesSumOverflow(first tree.Datum, second tree.Datum) bool {

[nit] I'd invert the result and rename to CanAddConstInts


pkg/sql/opt/norm/rules/limit.opt, line 70 at r3 (raw file):

    (Const $limit:* & (IsPositiveLimit $limit))
    $limitOrdering:* & (IsSameOrdering $offsetOrdering $limitOrdering) &
    ^(DoesSumOverflow $limit $offset)

[nit] indent this line (4 spaces)


pkg/sql/opt/norm/testdata/rules/limit, line 479 at r3 (raw file):


# Using MaxInt64. Do not apply rule when sum overflows.
opt expect-not=(PushLimitIntoOffset)

[nit] parens not necessary

Ridwan Sharif added 2 commits July 9, 2019 11:13
This change pushes the limit into an offset whenever possible.
This shouldn't worsen any plan but does allow the GetLimitedScans
rule to fire in more scenarios.

Fixes cockroachdb#30416.

Release note: None
This change adds an overflow check before applying the rule.

Release note: None
Copy link
Author

@ridwanmsharif ridwanmsharif left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

craig bot pushed a commit that referenced this pull request Jul 9, 2019
38570: opt: fix panic recovery for error handling r=RaduBerinde a=RaduBerinde

The major entry points in the optimizer catch all panics that throw an
error and converts them to errors. Unfortunately, this also catches
runtime errors (in which case we convert them to errors and lose the
stack trace).

This change adds a `ShouldCatch` helper which determines if we should
return a thrown object as an error. If the object is a
`runtime.Error`, it gets wrapped by an AssertionFailed error which
will cause correct error handling (stack trace, sentry reporting, etc).

As part of this change, we are also removing wrappers like
`builderError`, which are no longer useful. We fix the opt tester to
fail with the full error information (using `%+v`) for assertion
errors.

Release note: None

38660: opt: push limit into offset r=ridwanmsharif a=ridwanmsharif

This change pushes the limit into an offset whenever possible.
This shouldn't worsen any plan but does allow the `GetLimitedScans`
rule to fire in more scenarios.

Fixes #30416.
~~This is currently blocked on #38659.~~

Release note: None

38743: roachtest: skip jepsen/multi-register r=god a=nvanbenschoten

There's no use running this every night until #36431 is fixed.

Release note: None

38746: roachtest: don't reuse clusters after test failure r=andreimatei a=andreimatei

We've had a case where a cluster got messed up somehow and then a bunch
of tests that tried to reuse it failed. This patch employes a big hammer
and makes it so that we don't reuse a cluster after test failure (which
failure can be cluster related or not).

Release note: None

38766: scripts/release-notes.py: help the user with --from/--until r=lhirata a=knz

Requested by @lhirata

Release note: None

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Ridwan Sharif <ridwan@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jul 9, 2019

Build succeeded

@craig craig bot merged commit cdc709d into cockroachdb:master Jul 9, 2019
@ridwanmsharif ridwanmsharif deleted the push-limit-into-offset branch July 9, 2019 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: Order by Offset not using index
3 participants