Skip to content

Conversation

@madelynnblue
Copy link
Contributor

Geo costing is now the only function that multiplies its row count by
the filters cost. The other JOINs and SELECT will be added in another
PR since it is a more complicated change due to some plans changing,
which must come with some justification.

Fixes #48214

Release note: None

@madelynnblue madelynnblue requested a review from rytaft May 19, 2020 17:03
@madelynnblue madelynnblue requested a review from a team as a code owner May 19, 2020 17:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@madelynnblue
Copy link
Contributor Author

This PR collects all of the geo-related changes from #49074 into a single commit. That other PR will be closed and the non-geo changes can be done at some future time. They were getting hairy and needed some more data-backed justification.

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: (but wait for an ok from @rytaft too)

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


pkg/sql/opt/xform/coster.go, line 540 at r1 (raw file):

	cost += memo.Cost(rowsProcessed) * perRowCost

	// We don't add the result of computeFiltersCost to perRowCost because

[nit] Maybe explain why we're doing 1+ (we want a "set up cost" even for cases with very tiny row count).


pkg/sql/opt/xform/coster.go, line 583 at r1 (raw file):

		// Add a constant "setup" cost per ON condition to account for the fact that
		// the rowsProcessed estimate alone cannot effectively discriminate between
		// plans when RowCount is too small.

[nit] Add a TODO here that we should perhaps separate the one-time and the per-row cost and return them separately, to make the interface nicer. Currently this clashes conceptually with the (1+rowsProcessed) thing we're adding.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mjibson)


pkg/sql/opt/xform/coster.go, line 493 at r1 (raw file):

	// Take into account that the "internal" row count is higher, according to
	// the selectivities of the conditions. In particular, we need to ignore
	// left-over conditions that are not selective.

[nit] doesn't actually matter whether or not they are selective -- we still need to process the same number of rows. I'd say "we need to ignore the conditions that don't affect the number of rows processed".

Geo costing is now the only function that multiplies its row count by
the filters cost. The other JOINs and SELECT will be added in another
PR since it is a more complicated change due to some plans changing,
which must come with some justification.

Fixes #48214

Release note: None
Copy link
Contributor Author

@madelynnblue madelynnblue 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 (and 2 stale) (waiting on @RaduBerinde and @rytaft)


pkg/sql/opt/xform/coster.go, line 493 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] doesn't actually matter whether or not they are selective -- we still need to process the same number of rows. I'd say "we need to ignore the conditions that don't affect the number of rows processed".

Thanks for the words. It's taking a while for me to get it.


pkg/sql/opt/xform/coster.go, line 540 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Maybe explain why we're doing 1+ (we want a "set up cost" even for cases with very tiny row count).

The 1+ is documented as a setup cost in the comment for computeFiltersCost. Otherwise it'd be duplicated for each call of that function, which seems excessive.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @RaduBerinde)

@rytaft
Copy link
Collaborator

rytaft commented May 20, 2020


pkg/sql/opt/xform/coster.go, line 493 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Thanks for the words. It's taking a while for me to get it.

No worries! This stuff is definitely not obvious.

@madelynnblue
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented May 21, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented May 21, 2020

Build succeeded

@craig craig bot merged commit 148c315 into cockroachdb:master May 21, 2020
@madelynnblue madelynnblue deleted the geo-costs branch May 21, 2020 17:53
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.

opt: update coster to account for cost of executing geospatial functions

4 participants