fix: input function does not allow tailing spaces by yihong0618 · Pull Request #148 · pgvector/pgvector

yihong0618 · 2023-06-08T08:31:17Z

this fix #147

also add test case for it

passed CI can check: https://github.com/yihong0618/pgvector/actions/runs/5209060111

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

jkatz

This aligns with current PostgreSQL behavior with arrays, e.g.:

postgres=# SELECT '{1   , 2   }'::int[];
 int4  
-------
 {1,2}
(1 row)

However, I'd recommend following the convention that PostgreSQL has with the array_isspace function for the character checking (which would have to be reimplemented, as it's static):

https://github.com/postgres/postgres/blob/378d73ef204d0dcbeab834d52478e8cb90578ab7/src/backend/utils/adt/arrayfuncs.c#L438

Here is an example of how it is used in the array_in code:

https://github.com/postgres/postgres/blob/378d73ef204d0dcbeab834d52478e8cb90578ab7/src/backend/utils/adt/arrayfuncs.c#L257

yihong0618 · 2023-06-09T00:39:47Z

This aligns with current PostgreSQL behavior with arrays, e.g.:
postgres=# SELECT '{1   , 2   }'::int[];
 int4  
-------
 {1,2}
(1 row)
However, I'd recommend following the convention that PostgreSQL has with the array_isspace function for the character checking (which would have to be reimplemented, as it's static):

https://github.com/postgres/postgres/blob/378d73ef204d0dcbeab834d52478e8cb90578ab7/src/backend/utils/adt/arrayfuncs.c#L438

Here is an example of how it is used in the array_in code:

https://github.com/postgres/postgres/blob/378d73ef204d0dcbeab834d52478e8cb90578ab7/src/backend/utils/adt/arrayfuncs.c#L257

thanks will use array_isspace

@jkatz
fixed thanks for the info, and add front and end spaces support like pg array passed CI: https://github.com/yihong0618/pgvector/actions/runs/5217272065

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

ankane · 2023-06-09T01:51:22Z

src/vector.c

+ * https://github.com/postgres/postgres/blob/378d73ef204d0dcbeab834d52478e8cb90578ab7/src/backend/utils/adt/arrayfuncs.c#L438
+ */
+static inline bool
+array_isspace(char ch)


Let's name this vector_isspace

ankane · 2023-06-09T01:51:54Z

src/vector.c


+	/*
+	 * Note: we currently allow whitespace between, but not within,
+	 * dimension items.


I'm not sure this comment makes sense for vectors, so let's remove it

ankane

Hi @yihong0618, thanks for reporting and the PR! Looks great. Added a few comments inline (accidentally submitted the first review early).

ankane · 2023-06-09T01:57:55Z

src/vector.c

 		CheckElement(x[dim]);
 		dim++;
+		// stringEnd is space is OK to support SELECT '[   1  ,       2     ]'::vector(2);
+		while (array_isspace((unsigned char)*stringEnd))


No need to cast here or below

seems we need that, this is handle [1 , 2 ] which has spaces between 1 and ,

(unsigned char) shouldn't be needed. Also, I think it should come after the if (stringEnd == pt) check.

copy that, thanks for the info~

ankane · 2023-06-09T01:58:49Z

test/sql/input.sql

+SELECT '[1,2,3  ]'::vector;
+SELECT '[1,     2,    3  ]'::vector;
+SELECT '[1.   , 2, 3 ]'::vector(3);
+SELECT ' [1,2,3] '::vector(3);


I think we can combine this into a single test

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

yihong0618 · 2023-06-09T02:28:39Z

@ankane thanks very much for the review, the comments has been fixed now, and one CI on ubuntu seems failed,
I am not sure its flaky test will check: https://github.com/yihong0618/pgvector/actions/runs/5217584573/jobs/9417561780

rerun and passed

ankane · 2023-06-09T02:52:09Z

Awesome, big thanks @yihong0618!

yihong0618 · 2023-06-09T02:53:08Z

@ankane thanks for your great repo~

fix: input function does not allow tailing spaces

a2bb83d

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

jkatz reviewed Jun 8, 2023

View reviewed changes

fix: by review message, add front and end spaces support

3c33120

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

ankane reviewed Jun 9, 2023

View reviewed changes

yihong0618 added 2 commits June 9, 2023 10:20

fix: apply by review

4bac881

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

fix: drop useless cast

2b162bf

ankane merged commit 41c68bf into pgvector:master Jun 9, 2023

jkatz pushed a commit to jkatz/pgvector that referenced this pull request Jun 9, 2023

fix: input function does not allow tailing spaces (pgvector#148)

1118e28

Conversation

yihong0618 commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkatz left a comment

Choose a reason for hiding this comment

Uh oh!

yihong0618 commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihong0618 commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankane commented Jun 9, 2023

Uh oh!

yihong0618 commented Jun 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

yihong0618 commented Jun 8, 2023 •

edited

Loading

yihong0618 commented Jun 9, 2023 •

edited

Loading

yihong0618 commented Jun 9, 2023 •

edited

Loading