oldmoe / litestack

MIT License
1.02k stars 56 forks source link

Litesearch: ActiveRecord support for TEXT/STRING primary keys #112

Closed skiz closed 2 months ago

skiz commented 4 months ago

Given a schema with a TEXT primary key, litesearch cannot index these tables. This negates the ability to use this with UUIDS or other generated ids.

Attempting to dig in to the issues, I found that it also heavily relies on ROWID, so any tables using WITHOUT ROWID are also not able to be indexed (which generally are TEXT pkeys).

I may be able to look in to this further and add support, but also wanted to get some feedback in case theres no way it will be able to work, or has already been discussed.

diff --git a/test/test_ar_search.rb b/test/test_ar_search.rb
index 6309009..56e6bd0 100644
--- a/test/test_ar_search.rb
+++ b/test/test_ar_search.rb
@@ -19,6 +19,7 @@ db = ActiveRecord::Base.connection.raw_connection
 db.execute("CREATE TABLE authors(id INTEGER PRIMARY KEY, name TEXT, created_at TEXT, updated_at TEXT)")
 db.execute("CREATE TABLE publishers(id INTEGER PRIMARY KEY, name TEXT, created_at TEXT, updated_at TEXT)")
 db.execute("CREATE TABLE books(id INTEGER PRIMARY KEY, title TEXT, description TEXT, published_on TEXT, author_id INTEGER, publisher_id INTEGER, state TEXT, created_at TEXT, updated_at TEXT, active INTEGER)")
+db.execute("CREATE TABLE bar_codes(id TEXT PRIMARY KEY, book_id INTEGER, provider TEXT) WITHOUT ROWID")

 class ApplicationRecord < ActiveRecord::Base
   primary_abstract_class
@@ -45,6 +46,7 @@ end
 class Book < ApplicationRecord
   belongs_to :author
   belongs_to :publisher
+  has_many :bar_codes

   include Litesearch::Model

@@ -60,8 +62,19 @@ class Book < ApplicationRecord
   end
 end

+class BarCode < ApplicationRecord
+  belongs_to :book
+
+  include Litesearch::Model
+
+  BarCode.litesearch do |schema|
+    schema.fields [:provider]
+  end
+end
+
 class TestActiveRecordLitesearch < Minitest::Test
   def setup
+    BarCode.delete_all
     Book.drop_index!
     Book.delete_all
     Author.delete_all
@@ -85,6 +98,11 @@ class TestActiveRecordLitesearch < Minitest::Test
     Author.create(name: "Osama Penguin")
     Book.create(title: "In a middle of a night", description: "A tale of sleep", published_on: "2008-10-01", state: "available", active: true, publisher_id: 1, author_id: 1)
     Book.create(title: "In a start of a night", description: "A tale of watching TV", published_on: "2006-08-08", state: "available", active: false, publisher_id: 2, author_id: 1)
+    BarCode.create(id: "ABC123", provider: "Publishing Company", book_id: 1)
+  end
+
+  def test_barcode_index
+    BarCode.rebuild_index!
   end

   def test_similar
oldmoe commented 4 months ago

Hi @skiz,

Indeed Litesearch relies heavily on rowid, but I will be looking into relaxing this requirement and try to find a way to map composite keys as well

oldmoe commented 3 months ago

Hi @skiz I have been looking at this issue, and there doesn't seem to be an effective solution to it, at least not one that would be less expensive than having a rowid field in the original table

happy to discuss this in more detail if you like

gabriel-curtino commented 3 months ago

Hi!

I had the same issue here (the one in the subject, not the WITHOUT ROWID tables) and making liteserach rely in the rowid works for me: https://github.com/oldmoe/litestack/pull/117

oldmoe commented 2 months ago

Hi @gabriel-curtino, I was hesitant from relying on rowid because historically SQLite used to reassign these after a vacuum. This doesn't seem to be the case for recent versions, I will verify this and integrate this enhancement if it turns out rowids are stable now.

gabriel-curtino commented 2 months ago

Hi @gabriel-curtino, I was hesitant from relying on rowid because historically SQLite used to reassign these after a vacuum. This doesn't seem to be the case for recent versions, I will verify this and integrate this enhancement if it turns out rowids are stable now.

Interesting, I've missed that. I researched about the rowid after seeing it was being used for the backed schema triggers.

Wondering if rebuilding the index after vacuum would be an issue thinking that it might not be usual to vacuum the db used in a webapp and it's seems prettry straighforward unless you've some kind of complex standalone index.

I still think that it worth to support any kind of primary key (or no primary-key) over having to rebuild the index after vacuum.

oldmoe commented 2 months ago

I have merged the PR already, and I believe requiring a rebuild after a vacuum is something we can live with. Other than that, I don't think there is much we can do for without rowid tables, I will close this issue as not planned