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

roachtest.unoptimized-query-oracle/disable-rules=half #87023

Closed
rytaft opened this issue Aug 29, 2022 · 4 comments · Fixed by #87085
Closed

roachtest.unoptimized-query-oracle/disable-rules=half #87023

rytaft opened this issue Aug 29, 2022 · 4 comments · Fixed by #87085
Labels
branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). T-sql-queries SQL Queries Team

Comments

@rytaft
Copy link
Collaborator

rytaft commented Aug 29, 2022

roachtest.unoptimized-query-oracle/disable-rules=half failed with artifacts on master @ 524fd14da3fefcd849f44a835cc5f88f5dbdadcc:

		  |   	"-99,a\n,NULL,BOX(-1 15,0 16),0,{\"\\\\xc5b5f3991f524f\"},NULL,\ta",
		  |   	... // 15 identical elements
		  |   	"-99,a\n,NULL,BOX(-1.2958624195456403 -0.7007902262760126,-0.76394"...,
		  |   	"-99,a\n,NULL,BOX(-1.3207114753925442 0.07543733824577625,0.411722"...,
		  |   	strings.Join({
		  |   		"-99,a\n,NULL,BOX(-1.371471829151467 1.5445529591343243,-0.4097185",
		  |   		``8119740223 1.5844682297595416),0,{"\\x97","\\x4161"},``,
		  | + 		"NULL",
		  |   		``,	a``,
		  |   	}, ""),
		  |   	"-99,a\n,NULL,BOX(-1.3926406249344883 -0.6842762115529295,-0.23034"...,
		  |   	"-99,a\n,NULL,BOX(-1.3965146560734436 -0.5498867231573248,-0.99270"...,
		  |   	... // 35 identical elements
		  |   	"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x\",\"\\\\x2061\"},NULL,\ta",
		  |   	"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x\",\"\\\\x2f78f42ae5ad84f2\",\"\\"...,
		  |   	strings.Join({
		  |   		"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x\",\"\\\\x6120\",\"\\\\x\",\"\\\\x840c",
		  |   		"148fef14\"},\xc3",
		  | - 		"\x80",
		  | + 		"\xa0",
		  |   		",h\xc5",
		  |   	}, ""),
		  |   	"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x\",\"\\\\x616162\",\"\\\\xaf928150"...,
		  |   	"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x\",\"\\\\x7c3fdc0d748201\"},NUL"...,
		  |   	... // 55 identical elements
		  |   	"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x58\"},',\ta",
		  |   	"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x5b\"},\f,\ta",
		  |   	strings.Join({
		  |   		"-99,a\n,NULL,BOX(-10 -10,10 10),0,{\"\\\\x5f\",\"\\\\x4161\",\"\\\\xc3a0c3a1",
		  |   		``","\\x3a9d1d5c8d"},``,
		  | - 		"\x00",
		  |   		",aaaaaa",
		  |   	}, ""),
		  |   	... // 347 identical and 12 modified elements
		  |   }
		  | sql: SELECT
		  | 	(-99):::INT8 AS col_3616,
		  | 	e'a\n':::STRING AS col_3617,
		  | 	tab_1118.col1_0 AS col_3618,
		  | 	tab_1118.col1_7 AS col_3619,
		  | 	0:::OID AS col_3620,
		  | 	tab_1118.col1_1 AS col_3621,
		  | 	lag(tab_1118.col1_2::STRING) OVER (PARTITION BY tab_1118.col1_3, tab_1118.crdb_internal_mvcc_timestamp ORDER BY tab_1118.col1_16, tab_1118.col1_2 ROWS CURRENT ROW)::STRING
		  | 		AS col_3622,
		  | 	tab_1118.col1_6 AS col_3623
		  | FROM
		  | 	defaultdb.public.table1@[0] AS tab_1118
		  | ORDER BY
		  | 	tab_1118.col1_15 DESC
		Error types: (1) *withstack.withStack (2) *errutil.withPrefix (3) *withstack.withStack (4) *errutil.leafError

Parameters: ROACHTEST_cloud=gce , ROACHTEST_cpu=4 , ROACHTEST_ssd=0

Help

See: roachtest README

See: How To Investigate (internal)

This test on roachdash | Improve this report!

Originally posted by @cockroach-teamcity in #86308 (comment)

Jira issue: CRDB-19105

@blathers-crl
Copy link

blathers-crl bot commented Aug 29, 2022

Hi @rytaft, please add a C-ategory label to your issue. Check out the label system docs.

While you're here, please consider adding an A- label to help keep our repository tidy.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@rytaft rytaft added C-test-failure Broken test (automatically or manually discovered). branch-master Failures and bugs on the master branch. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Aug 29, 2022
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Aug 29, 2022
@rytaft rytaft changed the title roachtest.unoptimized-query-oracle/disable-rules=half [failed](https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Nightlies_RoachtestNightlyGceBazel/6231545?buildTab=log) with [artifacts](https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Nightlies_RoachtestNightlyGceBazel/6231545?buildTab=artifacts#/unoptimized-query-oracle/disable-rules=half) on master @ [524fd14da3fefcd849f44a835cc5f88f5dbdadcc](https://github.com/cockroachdb/cockroach/commits/524fd14da3fefcd849f44a835cc5f88f5dbdadcc): roachtest.unoptimized-query-oracle/disable-rules=half Aug 29, 2022
@DrewKimball
Copy link
Collaborator

DrewKimball commented Aug 29, 2022

It looks like the plans that generated the diff are the same - so it's probably vectorize on vs off that makes the difference, specifically for the lag column.

@DrewKimball
Copy link
Collaborator

DrewKimball commented Aug 29, 2022

I think I see the problem... in this query, the lag operator is taking the next row that orders greater than the current row. We make sure to add the lag input column to the ORDER BY clause, but consider what happens when there are at least two rows that order the same. Within that peer group, any order is allowed, but lag will output different values depending on whether the row happens to be the last one in its peer group (in which case it will output the next smallest/largest value as opposed to the current value).
I think it may be necessary to ensure we order on a key in cases like this one to prevent nondeterminism.

DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Aug 30, 2022
This commit ensures that sqlsmith adds all function argument columns to
the `ORDER BY` clause when nondetermistic functions are disabled. This is
necessary for functions like `string_agg`, for which there may be multiple
possible values for each argument when the order is not specified.

As a special case, `lead` and `lag` require all input columns to be added
to the `ORDER BY` clause. This is because while they respect the specified
order, they can still access any value in the partition. This means that we
need to uniquely specify which rows fall on the boundaries of each peer group.
We can ensure this is the case by ordering on a key, and we can trivially
order on a key by ordering on all columns.

Partially fixes cockroachdb#85318
Fixes cockroachdb#87023

Release justification: testing-only change

Release note: None
@mgartner mgartner removed the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Aug 30, 2022
@mgartner
Copy link
Collaborator

Good find! Sounds like it's not a release blocker then.

craig bot pushed a commit that referenced this issue Aug 31, 2022
86919: ui/cluster-ui: link session txn fingerprint ids to details page r=xinhaoz a=xinhaoz

This commit links the each txn fingerprint id from the list of cached
txn fingerprints in the session details page to the txn fingerprint's
details page. The global date range is also changed to the session's
start and end time on click in order to fetch and render the
transaction details.

Release justification: low risk update to existing functionality
Release note (ui change): in the sessions details page, users
can click on a txn fingerprint id from the list of cached
txn fingerprints to go to that transaction's details page. The
app will also change the selected date range to that of the session's
start and end time on click.


https://www.loom.com/share/0de478ccdae446bb8a09934d995ecbd1

87085: smith: add all argument columns to ORDER BY r=DrewKimball a=DrewKimball

This commit ensures that sqlsmith adds all function argument columns to
the `ORDER BY` clause when nondetermistic functions are disabled. This is
necessary for functions like `string_agg`, for which there may be multiple
possible values for each argument when the order is not specified.

As a special case, `lead` and `lag` require all input columns to be added
to the `ORDER BY` clause. This is because while they respect the specified
order, they can still access any value in the partition. This means that we
need to uniquely specify which rows fall on the boundaries of each peer group.
We can ensure this is the case by ordering on a key, and we can trivially
order on a key by ordering on all columns.

Partially addresses #85318
Fixes #87023

Release justification: testing-only change

Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: DrewKimball <[email protected]>
@craig craig bot closed this as completed in 599571c Aug 31, 2022
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants