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

feat: add sqllogictest #47

Merged
merged 12 commits into from
Nov 8, 2024
Merged

feat: add sqllogictest #47

merged 12 commits into from
Nov 8, 2024

Conversation

kemingy
Copy link
Member

@kemingy kemingy commented Nov 6, 2024

Signed-off-by: Keming <kemingyang@tensorchord.ai>
Signed-off-by: Keming <kemingyang@tensorchord.ai>
Signed-off-by: Keming <kemingyang@tensorchord.ai>
Signed-off-by: Keming <kemingyang@tensorchord.ai>
Signed-off-by: Keming <kemingyang@tensorchord.ai>
Signed-off-by: Keming <kemingyang@tensorchord.ai>
Signed-off-by: Keming <kemingyang@tensorchord.ai>
Signed-off-by: Keming <kemingyang@tensorchord.ai>
@kemingy
Copy link
Member Author

kemingy commented Nov 6, 2024

It's ready to review.

Since this lib depends on pgvector, I didn't include many basic logic tests here. Let me know if you want to add more @VoVAllen

cc @usamoi @cutecutecat

@kemingy kemingy requested a review from VoVAllen November 6, 2024 11:15
@VoVAllen
Copy link
Member

VoVAllen commented Nov 6, 2024

Some of the function (partition index/partial index/reindex concurrently) needs test verification. I don't know whether we support it for now. Fine to fail the test, but need verification

Signed-off-by: Keming <kemingyang@tensorchord.ai>
@kemingy
Copy link
Member Author

kemingy commented Nov 7, 2024

https://github.com/tensorchord/pgvecto.rs/blob/main/tests/sqllogictest/issue_427.slt https://github.com/tensorchord/pgvecto.rs/blob/main/tests/sqllogictest/null.slt https://github.com/tensorchord/pgvecto.rs/blob/main/tests/sqllogictest/partition.slt https://github.com/tensorchord/pgvecto.rs/blob/main/tests/sqllogictest/pushdown_plan.slt https://github.com/tensorchord/pgvecto.rs/blob/main/tests/sqllogictest/pushdown_range.slt https://github.com/tensorchord/pgvecto.rs/blob/main/tests/sqllogictest/reindex.slt

Some observations:

  • https://github.com/pgvector/pgvector?tab=readme-ov-file#vector-type doesn't support NaN or Infinity
  • unable to create an index on the partitioned table directly, but can use ONLY and then create indexes on each partitions
  • rabbithole only supports the f32 (pgvector type) index
  • rabbithole doesn't have fn/op like sphere, <<->>, etc. failed when calling <<->>
  • pgvector sparsevec index from 1
  • pgvector bit use text format like '010101110'
  • pgvector uses l2 distance instead of l2 squared distance

Some of the function (partition index/partial index/reindex concurrently) needs test verification. I don't know whether we support it for now. Fine to fail the test, but need verification

So null, pushdown_range, pushdown_plan failed.

@usamoi
Copy link
Contributor

usamoi commented Nov 7, 2024

unable to create an index on the partitioned table directly, but can use ONLY and then create indexes on each partitions

I think this should work?

rabbithole doesn't have fn/op like sphere, <<->>, etc

It does have.

pgvector uses l2 distance instead of l2 squared distance

be fixed by #44

@kemingy
Copy link
Member Author

kemingy commented Nov 7, 2024

unable to create an index on the partitioned table directly, but can use ONLY and then create indexes on each partitions

I think this should work?

There will be:

ERROR:  operator class "vector_dot_ops" does not exist for access method "rabbithole"

Though the search_path is correctly configured.

rabbithole doesn't have fn/op like sphere, <<->>, etc

It does have.

Yes, it has. But there will be an error when using the <<->>:

ERROR:  calling `type_oid` is never expected

@usamoi
Copy link
Contributor

usamoi commented Nov 7, 2024

Though the search_path is correctly configured.

Are you testing with PostgreSQL 17? It's a bug of PostgreSQL 17.

https://www.postgresql.org/message-id/18637-f51e314546e3ba2a%40postgresql.org

@kemingy
Copy link
Member Author

kemingy commented Nov 7, 2024

Though the search_path is correctly configured.

Are you testing with PostgreSQL 17? It's a bug of PostgreSQL 17.

https://www.postgresql.org/message-id/18637-f51e314546e3ba2a%40postgresql.org

Weird.

While CREATE INDEX is running, the search_path is temporarily changed to pg_catalog, pg_temp.

Is that true? I tried public.vector_dot_ops in pg17 but also failed.

@usamoi
Copy link
Contributor

usamoi commented Nov 7, 2024

Is that true? I tried public.vector_dot_ops in pg17 but also failed.

I think you should try rabbithole.vector_dot_ops?

Signed-off-by: Keming <kemingyang@tensorchord.ai>
@kemingy
Copy link
Member Author

kemingy commented Nov 7, 2024

Is that true? I tried public.vector_dot_ops in pg17 but also failed.

I think you should try rabbithole.vector_dot_ops?

Fixed.

Is there a bug in <<->> op?

@usamoi
Copy link
Contributor

usamoi commented Nov 7, 2024

Is there a bug in <<->> op?

Yes. It's not expected behavior.

@VoVAllen
Copy link
Member

VoVAllen commented Nov 7, 2024

Can you try also rabbithole.<<->>?

@kemingy
Copy link
Member Author

kemingy commented Nov 7, 2024

Can you try also rabbithole.<<->>?

I guess you mean OPERATOR(rabbithole.<<->>). No, it doesn't work. This is totally different problems.

VoVAllen and others added 2 commits November 7, 2024 22:27
Signed-off-by: Keming <kemingyang@tensorchord.ai>
@kemingy
Copy link
Member Author

kemingy commented Nov 8, 2024

@VoVAllen review?

@kemingy kemingy merged commit 31aed43 into main Nov 8, 2024
6 checks passed
@kemingy kemingy deleted the sql branch November 8, 2024 03:53
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.

test: Migrate sql test in pgvecto.rs to this repo
3 participants