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/opbench: add more cost tests #36390

Merged
merged 1 commit into from
Apr 2, 2019
Merged

Conversation

justinj
Copy link
Contributor

@justinj justinj commented Apr 1, 2019

This commit introduces more costing tests for some scans and sorts.

The plots some some room for improvement on the costing:

image

Release note: None

@justinj justinj requested review from rytaft and RaduBerinde April 1, 2019 19:26
@justinj justinj requested a review from a team as a code owner April 1, 2019 19:26
@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.

:lgtm:

Cool graph. I wonder what metric would help bring them more in-line. Maybe avg value size for each column.

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

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 @justinj)


pkg/sql/opt/opbench/opbench_test.go, line 552 at r1 (raw file):

				(Table "%s")
				(Cols "$cols")
		  	(HardLimit $rows)

[nit] this isn't lined up


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

		},
	}
}

Seems like there's a lot of common code between makeScanSpec and makeSortSpec -- could some of it be put into a helper function?


pkg/sql/opt/opbench/opbench_test.go, line 585 at r1 (raw file):

}

// SortLineitemSpec scans the lineitem table.

scans -> scans and sorts

This commit introduces more costing tests for some scans and sorts.

Release note: None
Copy link
Contributor Author

@justinj justinj left a comment

Choose a reason for hiding this comment

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

Thanks! I think avg value size would help, but I think there's an easier win which is tweaking the cost of a single column, since there's a moderately high fixed cost to that. I want to lay down some foundation for talking about the accuracy here before making real changes.

bors r+

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


pkg/sql/opt/opbench/opbench_test.go, line 552 at r1 (raw file):

Previously, rytaft wrote…

[nit] this isn't lined up

Done.


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

Previously, rytaft wrote…

Seems like there's a lot of common code between makeScanSpec and makeSortSpec -- could some of it be put into a helper function?

Pulled out the part that builds the list of columns.


pkg/sql/opt/opbench/opbench_test.go, line 585 at r1 (raw file):

Previously, rytaft wrote…

scans -> scans and sorts

Done.

craig bot pushed a commit that referenced this pull request Apr 2, 2019
36390: opt/opbench: add more cost tests r=justinj a=justinj

This commit introduces more costing tests for some scans and sorts.

The plots some some room for improvement on the costing:

![image](https://user-images.githubusercontent.com/409075/55353842-428ce880-5492-11e9-8c11-f85de0c5ca11.png)


Release note: None

Co-authored-by: Justin Jaffray <[email protected]>
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 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale)

@craig
Copy link
Contributor

craig bot commented Apr 2, 2019

Build succeeded

@craig craig bot merged commit 9ff94be into cockroachdb:master Apr 2, 2019
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.

4 participants