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: sql: st_expand with equal returns incorrect result? #93541

Closed
cockroach-teamcity opened this issue Dec 13, 2022 · 5 comments · Fixed by #105789
Closed

roachtest: sql: st_expand with equal returns incorrect result? #93541

cockroach-teamcity opened this issue Dec 13, 2022 · 5 comments · Fixed by #105789
Assignees
Labels
branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-roachtest O-robot Originated from a bot. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Milestone

Comments

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Dec 13, 2022

roachtest.unoptimized-query-oracle/disable-rules=all/rand-tables failed with artifacts on master @ a80652b2e4691ea76ea49e797b1b9e0998e1d61f:

+ 		"58c02adfe3fcdb6650401c1bda439d8d0142f79217d42ea163c0c06922c601db",
+ 		"febf3c1fe803e34eff410103000040010000000700000070beab68857861c073",
+ 		"45165828b453c0d835d3fde9a1d4c1b848c51b7a7966401e10a4c4d97e3ec07a",
+ 		"ca47a0e48001c23c8a4c11c11762409cc88c4272714540d03f3cb25e9cc4c196",
+ 		"d637cb9c714bc0901ad4e9efb52b40cedfaf7593fce3c139a72dd60f1c5cc080",
+ 		"25b72031942440d4df95c2c770fc41bb605f69ff6d5cc020f89300912306c0f8",
+ 		"f468bdd8dcf84170beab68857861c07345165828b453c0d835d3fde9a1d4c101",
+ 		"06000060e610000006000000010300004001000000040000002070072fbbf65c",
+ 		"40c84743a765e650c0846e2aa0a479f0c1747e9338086a4d4000e74610c97d51",
+ 		"4064759afd9babf9c18088b686d3c83140b05c9fd0d4bf47405856d7b95dbdfe",
+ 		"c12070072fbbf65c40c84743a765e650c0846e2aa0a479f0c101030000400100",
+ 		"00000500000070d461aae79a5840f40c6e7e40493d402e0f766e460d0142442c",
+ 		"f9ee961d5f40b08266261b263840869db977a6ebfec1ee2ccf9d90a96240aae5",
+ 		"f2e9f54c5540d22753387e81f1c1c0235865106244c0365e99f4a8175340a409",
+ 		"4d651adde2c170d461aae79a5840f40c6e7e40493d402e0f766e460d01420103",
+ 		"0000400100000008000000345feaf0953360c0aa49ac9397423ec000e9152103",
+ 		"6dcec1d5c8ef7d489061c0f683bc7a76493fc000a05ab3beb6a941264daa4a74",
+ 		"9064c0cb72995178c244c0b0b9b4bb5745cdc198f587e68a404840430b47cbad",
+ 		"3f50c0801a0a61de61d2c15486625cdd244640cb170d008ce14dc033ca7423ce",
+ 		"8df4c1962f8621ba9a6140983649d987ff2cc00121f51e8634f4c184c8a77275",
+ 		"f94bc038a4166774cf5540bc6c1ed0fd3af7c1345feaf0953360c0aa49ac9397",
+ 		"423ec000e91521036dcec101030000400100000006000000b0440a56695159c0",
+ 		"506272905c134940e6ceb3287dadf341b8a9b469af0d35c0bc280be653543340",
+ 		"fd7a8b658f5ff2c1dcc9a2781e3d5940467a82b4ecd4534058d103fa4309ef41",
+ 		"9093c07c58685c406ae353d773aa554048c8eef0f0e6fe4166e06abcd92b52c0",
+ 		"a457a6f9cab75240502bd9307f91dac1b0440a56695159c0506272905c134940",
+ 		"e6ceb3287dadf34101030000400100000005000000d830fb769385334000ce68",
+ 		"8285fc49c0d8f60c2a23a0f6c19caf714bbb695340666a0856dd4742c07c7e2f",
+ 		"fb9293ef4193cf67655e7450c0702c3b97a85930c010db8e8bf46b01c2a1dfe4",
+ 		"5c438064c038ee30841e2b34c038012f0c0690ff41d830fb769385334000ce68",
+ 		"8285fc49c0d8f60c2a23a0f6c101030000400100000008000000aa57710fad41",
+ 		"5ec0d7ba1ac0a11455c0c36c929c6f77f8c11c838a39431a5f40ed07dbee94cd",
+ 		"55c0dc5f965fcb1bf9c13a77ff3c789963404ed5fb7c9fd751c0044169a42cd2",
+ 		"e741a03bcf1f941d5d40a788442c6e884bc0709a107f16f9cec1c0cbaac24a4f",
+ 		"0640f8927923913144409c10812de5e0e3c1407fc9dc79521cc0dd0f906dbd44",
+ 		"45c07a26408e5504f9c119ac99197c5865c09c6349e01b4836c038487e50c737",
+ 		"0242aa57710fad415ec0d7ba1ac0a11455c0c36c929c6f77f8c",
  		... // 1 identical byte
  	}, ""),
  	"01070000a0e61000000000000001070000a0e61000000000000001070000a0e6"...,
  	"01070000a0e61000000300000001060000a0e610000001000000010300008001"...,
  }
sql: SELECT
	string_agg(tab_1548.col1_9::STRING, tab_1548.col1_9::STRING ORDER BY tab_1548.col1_9::STRING ASC, tab_1548.col1_9::STRING ASC)::STRING
		AS col_4552
FROM
	defaultdb.public.table1 AS tab_1547
	JOIN defaultdb.public.table1 AS tab_1548 ON (tab_1547.col1_7) = (tab_1548.col1_7)
GROUP BY
	tab_1548.col1_9

Parameters: ROACHTEST_cloud=gce , ROACHTEST_cpu=4 , ROACHTEST_encrypted=false , ROACHTEST_fs=ext4 , ROACHTEST_localSSD=true , ROACHTEST_ssd=0

Help

See: roachtest README

See: How To Investigate (internal)

/cc @cockroachdb/sql-queries

This test on roachdash | Improve this report!

Jira issue: CRDB-22388

@cockroach-teamcity cockroach-teamcity added branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-roachtest O-robot Originated from a bot. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Dec 13, 2022
@cockroach-teamcity cockroach-teamcity added this to the 23.1 milestone Dec 13, 2022
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Dec 13, 2022
@cockroach-teamcity
Copy link
Member Author

roachtest.unoptimized-query-oracle/disable-rules=all/rand-tables failed with artifacts on master @ 94e03c016955dfd64d4e15358ea92226a8f362aa:

test artifacts and logs in: /artifacts/unoptimized-query-oracle/disable-rules=all/rand-tables/run_1
(test_impl.go:291).Fatal: . 2033 statements run: expected unoptimized and optimized results to be equal
  []string(
- 	nil,
+ 	{
+ 		"'Uae',aaaaaa,-2000-01-01 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,-2000-01-01 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,-2000-01-01 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,-4713-11-24 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,-4713-11-24 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,-4713-11-24 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,0001-01-01 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,0001-01-01 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,0001-01-01 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,1942-08-15 01:23:56 +0000 +0000",
+ 		"'Uae',aaaaaa,1942-08-15 01:23:56 +0000 +0000",
+ 		"'Uae',aaaaaa,1942-08-15 21:13:20 +0000 +0000",
+ 		"'Uae',aaaaaa,1942-08-16 00:00:00 +0000 +0000",
+ 		"'Uae',aaaaaa,1942-08-16 00:08:20 +0000 +0000",
+ 		"'Uae',aaaaaa,1967-04-06 01:23:56 +0000 +0000",
+ 		"'Uae',aaaaaa,1967-04-06 21:13:20 +0000 +0000", ...,
+ 	},
  )
sql: SELECT
	'''Uae''':::TSQUERY AS col_3514, 'aaaaaa':::STRING AS col_3515, tab_1265.col2_2 AS col_3516
FROM
	defaultdb.public.table2@[0] AS tab_1265
WHERE
	'BOX(-10 -10,10 10)':::BOX2D::BOX2D
	IN (
			SELECT
				st_expand('BOX(0.8449703663020123 -0.9848842927527918,1.4327441619057475 -0.2409153334782671)':::BOX2D::BOX2D, tab_1266.col2_1::FLOAT8)::BOX2D::BOX2D
					AS col_3513
			FROM
				defaultdb.public.table2@[0] AS tab_1266
		)

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

Help

See: roachtest README

See: How To Investigate (internal)

This test on roachdash | Improve this report!

@mgartner
Copy link
Collaborator

I can't reproduce the first failure.

@mgartner
Copy link
Collaborator

Reduced reproduction of the second failure:

CREATE TABLE t (d INT PRIMARY KEY, f FLOAT4);

INSERT INTO t VALUES (1, 1.0), (2, 'NaN');

SET testing_optimizer_disable_rule_probability = 1.000000;

SELECT count(*) FROM t
WHERE 'BOX(-10 -10,10 10)'::BOX2D IN (
  SELECT st_expand('BOX(1 -1, 1 -1)'::BOX2D, t2.f) FROM t t2
);

RESET testing_optimizer_disable_rule_probability;

SELECT count(*) FROM t
WHERE 'BOX(-10 -10,10 10)'::BOX2D IN (
  SELECT st_expand('BOX(1 -1, 1 -1)'::BOX2D, t2.f) FROM t t2
);

The first SELECT returns 0 and the second returns 2. The correct result is 0 (confirmed with Postgres).

The query plan of the first (good result) is:

  scalar-group-by
   ├── columns: count:10
   ├── cardinality: [1 - 1]
   ├── immutable
   ├── stats: [rows=1]
   ├── cost: 3327.20667
   ├── key: ()
   ├── fd: ()-->(10)
   ├── distribution: us-east1
   ├── project
   │    ├── immutable
   │    ├── stats: [rows=333.3333]
   │    ├── cost: 3323.84333
   │    ├── distribution: us-east1
   │    └── select
   │         ├── columns: t.d:1 t.f:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
   │         ├── immutable
   │         ├── stats: [rows=333.3333]
   │         ├── cost: 3320.49
   │         ├── key: (1)
   │         ├── fd: (1)-->(2-4)
   │         ├── distribution: us-east1
   │         ├── scan t
   │         │    ├── columns: t.d:1 t.f:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
   │         │    ├── stats: [rows=1000, distinct(1)=1000, null(1)=0]
   │         │    ├── cost: 1145.22
   │         │    ├── key: (1)
   │         │    ├── fd: (1)-->(2-4)
   │         │    └── distribution: us-east1
   │         └── filters
   │              └── any: eq [immutable, subquery]
   │                   ├── project
   │                   │    ├── columns: st_expand:9
   │                   │    ├── immutable
   │                   │    ├── stats: [rows=1000]
   │                   │    ├── cost: 1165.24
   │                   │    ├── scan t [as=t2]
   │                   │    │    ├── columns: t2.d:5 t2.f:6 t2.crdb_internal_mvcc_timestamp:7 t2.tableoid:8
   │                   │    │    ├── stats: [rows=1000]
   │                   │    │    ├── cost: 1145.22
   │                   │    │    ├── key: (5)
   │                   │    │    └── fd: (5)-->(6-8)
   │                   │    └── projections
   │                   │         └── st_expand('BOX(1 -1,1 -1)', t2.f:6) [as=st_expand:9, outer=(6), immutable]
   │                   └── 'BOX(-10 -10,10 10)'
   └── aggregations
        └── count-rows [as=count_rows:10]

And the query plan of the second (bad result) is:

  scalar-group-by
   ├── columns: count:10
   ├── cardinality: [1 - 1]
   ├── immutable
   ├── stats: [rows=1]
   ├── cost: 2218.90333
   ├── key: ()
   ├── fd: ()-->(10)
   ├── distribution: us-east1
   ├── prune: (10)
   ├── select
   │    ├── immutable
   │    ├── stats: [rows=333.3333]
   │    ├── cost: 2215.54
   │    ├── distribution: us-east1
   │    ├── scan t
   │    │    ├── stats: [rows=1000]
   │    │    ├── cost: 1064.42
   │    │    └── distribution: us-east1
   │    └── filters
   │         └── exists [immutable, subquery]
   │              └── limit
   │                   ├── columns: st_expand:9
   │                   ├── cardinality: [0 - 1]
   │                   ├── immutable
   │                   ├── stats: [rows=1]
   │                   ├── cost: 141.089999
   │                   ├── key: ()
   │                   ├── fd: ()-->(9)
   │                   ├── select
   │                   │    ├── columns: st_expand:9
   │                   │    ├── immutable
   │                   │    ├── stats: [rows=10, distinct(9)=1, null(9)=0]
   │                   │    ├── cost: 141.069999
   │                   │    ├── fd: ()-->(9)
   │                   │    ├── limit hint: 1.00
   │                   │    ├── project
   │                   │    │    ├── columns: st_expand:9
   │                   │    │    ├── immutable
   │                   │    │    ├── stats: [rows=1000, distinct(9)=100, null(9)=0]
   │                   │    │    ├── cost: 140.039999
   │                   │    │    ├── limit hint: 100.00
   │                   │    │    ├── scan t [as=t2]
   │                   │    │    │    ├── columns: t2.f:6
   │                   │    │    │    ├── stats: [rows=1000, distinct(6)=100, null(6)=10]
   │                   │    │    │    ├── cost: 120.019999
   │                   │    │    │    ├── limit hint: 100.00
   │                   │    │    │    └── prune: (6)
   │                   │    │    └── projections
   │                   │    │         └── st_expand('BOX(1 -1,1 -1)', t2.f:6) [as=st_expand:9, outer=(6), immutable]
   │                   │    └── filters
   │                   │         └── st_expand:9 = 'BOX(-10 -10,10 10)' [outer=(9), constraints=(/9: [/'BOX(-10 -10,10 10)' - /'BOX(-10 -10,10 10)']; tight), fd=()-->(9)]
   │                   └── 1
   └── aggregations
        └── count-rows [as=count_rows:10]

The query plans look fine to me at a glance. Maybe there is a bug in how we evaluate st_expand:9 = 'BOX(-10, -10,10 10)' which is avoid in the first plan because we use an any: eq expression instead?

@mgartner mgartner changed the title roachtest: unoptimized-query-oracle/disable-rules=all/rand-tables failed roachtest: sql: st_expand with equal returns incorrect result? Dec 20, 2022
@mgartner
Copy link
Collaborator

I've confirmed the bug is present all the back in v20.2 when geo-spatial types were added. Removing the release-blocker label.

@mgartner mgartner removed the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Dec 20, 2022
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Jun 8, 2023
@rharding6373 rharding6373 self-assigned this Jun 27, 2023
@rharding6373
Copy link
Collaborator

I've reduced the difference down to:

defaultdb>SELECT 'BOX(NaN NaN,NaN NaN)'::BOX2D = 'BOX(-10 -10,10 10)'::BOX2D;                                   
  ?column?
------------
     t

On postgres:

rharding=# SELECT 'BOX(NaN NaN,NaN NaN)'::BOX2D = 'BOX(-10 -10,10 10)'::BOX2D;
 ?column? 
----------
 f

This appears to be a comparison issue involving NaNs with the BOX2D type. Only the "bad" plan in #93541 (comment) ends up performing this operation.

craig bot pushed a commit that referenced this issue Jun 29, 2023
105789: geo: fix nan handling in bounding box comparison r=otan a=rharding6373

Go always returns false when comparing float NaNs, but SQL expects NaNs to be less than any other float value. Before this change, the geo package's `CartesianBoundingBox` did not have special handling for NaNs, so it implemented the go behavior, which is incorrect for our use case.

This change adds correct NaN comparison behavior to bounding boxes.

Epic: None
Fixes: #93541
Fixes: #102661

Release note (bug fix): Fixes a bug in the geospatial cartesian bounding box type that had incorrect behavior when comparing boxes with NaN values.

Co-authored-by: rharding6373 <[email protected]>
@craig craig bot closed this as completed in 5292ef7 Jun 29, 2023
blathers-crl bot pushed a commit that referenced this issue Jun 29, 2023
Go always returns false when comparing float NaNs, but SQL expects NaNs
to be less than any other float value. Before this change, the geo
package's `CartesianBoundingBox` did not have special handling for NaNs,
so it implemented the go behavior, which is incorrect for our use case.

This change adds correct NaN comparison behavior to bounding boxes.

Epic: None
Fixes: #93541
Fixes: #102661

Release note (bug fix): Fixes a bug in the geospatial cartesian bounding
box type that had incorrect behavior when comparing boxes with NaN
values.
blathers-crl bot pushed a commit that referenced this issue Jun 29, 2023
Go always returns false when comparing float NaNs, but SQL expects NaNs
to be less than any other float value. Before this change, the geo
package's `CartesianBoundingBox` did not have special handling for NaNs,
so it implemented the go behavior, which is incorrect for our use case.

This change adds correct NaN comparison behavior to bounding boxes.

Epic: None
Fixes: #93541
Fixes: #102661

Release note (bug fix): Fixes a bug in the geospatial cartesian bounding
box type that had incorrect behavior when comparing boxes with NaN
values.
@exalate-issue-sync exalate-issue-sync bot removed the T-sql-queries SQL Queries Team label Jun 29, 2023
@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). O-roachtest O-robot Originated from a bot. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants