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

sql: NULL geography concatenation is inconsistent with Postgres #79846

Closed
mgartner opened this issue Apr 12, 2022 · 6 comments
Closed

sql: NULL geography concatenation is inconsistent with Postgres #79846

mgartner opened this issue Apr 12, 2022 · 6 comments
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. good first issue T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@mgartner
Copy link
Collaborator

mgartner commented Apr 12, 2022

From #67791 (comment):

In Postgres 14.2:

marcus=# SELECT NULL::GEOGRAPHY || NULL::GEOGRAPHY[] || NULL::GEOGRAPHY;
  ?column?
-------------
 {NULL:NULL}

In CRDB 21.2.7:

defaultdb> SELECT NULL::GEOGRAPHY || NULL::GEOGRAPHY[] || NULL::GEOGRAPHY;
   ?column?
---------------
  {NULL,NULL}

Jira issue: CRDB-15892

@mgartner mgartner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 12, 2022
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Apr 12, 2022
@otan
Copy link
Contributor

otan commented Apr 13, 2022

i wonder if this is just a display difference with non-postgres defined types.

HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
otan=# SELECT NULL::GEOGRAPHY || NULL::GEOGRAPHY[] || NULL::GEOGRAPHY || NULL::GEOGRAPHY;
     ?column?
------------------
 {NULL:NULL:NULL}
(1 row)

otan=# SELECT NULL::int || NULL::int[] || NULL::int || NULL::int;
     ?column?
------------------
 {NULL,NULL,NULL}
(1 row)

otan=# SELECT array_length(NULL::GEOGRAPHY || NULL::GEOGRAPHY[] || NULL::GEOGRAPHY || NULL::GEOGRAPHY, 1);
 array_length
--------------
            3
(1 row)

otan=# select ( NULL::int || NULL::int[] || NULL::int || NULL::int)::text;
       text
------------------
 {NULL,NULL,NULL}
(1 row)

otan=# SELECT (NULL::GEOGRAPHY || NULL::GEOGRAPHY[] || NULL::GEOGRAPHY || NULL::GEOGRAPHY)::text;
       text
------------------
 {NULL:NULL:NULL}
(1 row)

@otan
Copy link
Contributor

otan commented Apr 13, 2022

TIL about typdelim from pg_type, which is : for _geography:

otan=# select typdelim from pg_type where typname = '_geography';
 typdelim
----------
 :
(1 row)

spooky:

otan=# select typname, typdelim from pg_type where typdelim <> ',';
  typname   | typdelim
------------+----------
 box        | ;
 _box       | ;
 geometry   | :
 _geometry  | :
 geography  | :
 _geography | :
(6 rows)

@otan
Copy link
Contributor

otan commented Apr 13, 2022

seems like there's to changes to make here:

  • make a method Delimiter in the types package, which returns ; for box, : for geom/geog and , for everything else
  • change the delimiter for Format for DArray:
    comma = ","
  • change pg_type table to return the correct typdelim:
    var pgCatalogTypeTable = virtualSchemaTable{
  • add the relevant logic tests

@asing1
Copy link

asing1 commented Apr 15, 2022

@otan I can start looking at it. Please assign it to me.

@surahman
Copy link
Contributor

surahman commented Jun 1, 2022

@otan I have the PR underway but I am unsure of the changes that must happen for the pgCatalogTypeTable. Could you please elaborate I am new to the code base?

@otan
Copy link
Contributor

otan commented Jun 2, 2022

Thanks for looking @surahman ! i've added some comments in #82304 (comment)

craig bot pushed a commit that referenced this issue Jun 6, 2022
82304: [CRDB-15892] SQL: NULL geography concatenation is inconsistent with Postgres r=otan a=surahman

Github Issue [#79846](#79846)
JIRA ticket [CRDB-15892](https://cockroachlabs.atlassian.net/browse/CRDB-15892)


- [x] `Delimiter` in `sql/types/`:  `Box2D` `;` , for `Geometry`  and Geography` `:`, and for everything else `,`
- [x] Change the delimiter for `Format` for [`DArray`](https://github.com/cockroachdb/cockroach/blob/b9a675266303528d71b388ac35dfb87f145cc7a8/pkg/sql/sem/tree/datum.go#L4414)
- [x] Change `pg_type` table to return the correct `typdelim` in [`pgCatalogTypeTable`](https://github.com/cockroachdb/cockroach/blob/4c32814a93a9447abcc12c168a3a45a865cd3785/pkg/sql/pg_catalog.go#L2938)
- [ ] Add the relevant logic tests.
- [ ] Fix broken tests in `TestRandParseDatumStringAs` and `TestAvroSchema`.
- [ ] Add [test](https://github.com/cockroachdb/cockroach/blob/1a707a1/pkg/sql/logictest/testdata/logic_test/geospatial) for concatenating array components together.

`@otan`


Co-authored-by: Saad Ur Rahman <[email protected]>
@otan otan closed this as completed Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. good first issue T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

No branches or pull requests

5 participants