Refactor Talk indexing into a Talk::Index record. #540

kaspth · 2024-12-22T22:17:13Z

This does take some extra work since Active Record can't detect rowid is the primary key.

But with some awareness around that, it's pretty smooth sailing and we can entirely remove a lot of the custom SQL with standard Active Record.

kaspth · 2024-12-22T22:18:13Z

app/models/talk/searchable.rb

    end

    after_create_commit :create_in_index
    after_update_commit :update_in_index
-    after_destroy_commit :remove_from_index


We get this for free now via dependent: :destroy!

kaspth · 2024-12-22T23:54:02Z

I don't get what's going on in the tests.

In development the query looks correct and finds the talk:

rubyvideo(dev)> Talk.ft_search "Hotwire Cookbook"
  Talk Load (2.3ms)  SELECT "talks".* FROM "talks" INNER JOIN "talks_search_index" ON "talks_search_index"."rowid" = "talks"."id" WHERE (talks_search_index match 'Hotwire* Cookbook*') /* loading for pp */ LIMIT 11 /*application='Rubyvideo'*/

But in tests the query looks different and doesn't find the talk:

SELECT "talks".* FROM "talks" INNER JOIN "talks_search_index" ON "talks_search_index"."rowid" = "talks"."id" WHERE (talks_search_index match ?)  [[nil, "Hotwire* Cookbook*"]]

The [[nil, "Hotwire* Cookbook*"]] structure seems to indicate a bind parameter and potentially a prepared statement, but we don't seem to have that turned on/off anywhere. Maybe it's a Rails default somehow?

Swapping to where("#{table_name} match :query", query:) also doesn't work. Hm, I'm confused.

kaspth · 2024-12-22T23:57:36Z

Ok, if I put prepared_statements: false in database.yml, I get the same query as in development:

SELECT "talks".* FROM "talks" INNER JOIN "talks_search_index" ON "talks_search_index"."rowid" = "talks"."id" WHERE (talks_search_index match 'Hotwire* Cookbook*')

But it still doesn't find the book.

kaspth · 2024-12-23T00:13:14Z

Ah, something seems to be up when we try to insert the record, hm. It's missing the rowid here.

INSERT INTO "talks_search_index" ("title", "summary", "speaker_names") VALUES ('yo', '', 'Yauhen Karatkou') RETURNING "rowid" /*application='Rubyvideo'*/

kaspth · 2024-12-23T00:29:38Z

Ok, wow, maybe this is more trouble than it's worth. We have to drop back into raw SQL to get Active Record to put in the rowid when inserting.

adrienpoly · 2024-12-24T07:48:14Z

I like how it all feels really ActiveRecord.

kaspth · 2025-01-04T22:12:04Z

I'm hoping to get back to this, because I'd like to wrap it up.

This does take some extra work since Active Record can't detect `rowid` is the primary key. But with some awareness around that, it's pretty smooth sailing and we can entirely remove a lot of the custom SQL with standard Active Record.

…h these

…m SQL

kaspth · 2025-01-20T02:13:55Z

app/models/talk/index.rb

+  end
+
+  def reindex
+    update! id: talk.id, title: talk.title, summary: talk.summary, speaker_names: talk.speaker_names


We're taking advantage of Active Record automatically generating an update or insert statement based on new_record?.

Internally, update! calls create_or_update which then calls new_record? ? _create_record(&block) : _update_record(&block).

This also means we now take advantage of Active Record skipping needless update queries when there's no change to the attributes.

kaspth · 2025-01-20T02:17:08Z

app/models/active_record/sqlite/index.rb

+  extend ActiveSupport::Concern
+
+  included do
+    self.ignored_columns = [table_name.to_sym, :rank] # Rails parses our virtual table with these extra non-attributes.


A SQLite fts5 table like this is built on a rowid primary key and several columns to index. Active Record now understands these types of virtual tables but it doesn't recognize the rowid at all, not even as a column. (It's probably because the column isn't spelled out anywhere in the schema statements so Active Record has no column to parse there).

So this module encapsulates all the work we need to have Active Record recognize the rowid column as a column and as the primary key.

Oh yeah, and then Active Record also parses the table name and something called rank as valid columns, so we need to ignore those since they're internal to the fts5 implementation.

kaspth · 2025-01-20T02:18:24Z

app/models/active_record/sqlite/index.rb

+
+  private
+
+  def attributes_for_create(attribute_names)


We don't need to override attributes_for_update, because the rowid is included in the query_constraints_hash method, that Active Record calls in _update_record or wherever it was. The details are a little fuzzy to me now because of all the code I had to sift through.

kaspth · 2025-01-20T02:20:42Z

app/models/talk/searchable.rb

-    def select_snippet(column, offset, tag: "mark", omission: "…", limit: 32)
-      select("snippet(talks_search_index, #{offset}, '<#{tag}>', '</#{tag}>', '#{omission}', #{limit}) AS #{column}_snippet")
+    def reindex_all
+      Talk::Index.delete_all


@adrienpoly reading this again I'm confused as to why we need to delete the existing ones beforehand? Do you remember?

no I don't remember. Maybe I did that initially to just make sure we get something clean knowing that our data is verylimieted so doing a full rebuild is not really costly

@adrienpoly ah got it. Yeah, I think with this change we could let Active Record decide and spare some needless queries in case there's no update needed.

Suggested change

Talk::Index.delete_all

kaspth · 2025-01-20T02:23:38Z

@adrienpoly @marcoroth think this is ready now. The ActiveRecord::SQLite::Index module I added should make this easier to port to the Speaker's index table.

Follow-up to adrienpoly#540

adrienpoly · 2025-01-22T14:19:37Z

Thanks I will try it on my db and play a bit with it

Follow-up to adrienpoly#540

adrienpoly

@kaspth I tested it locally and it works great. I pushed a small extra test as I was playing with it.

Overall I like how we are getting closer to the ActiveRecord native API. It is clearly code that I wouldn't have been able to write. At first I was a bit concerned about maintenance in the future. but now that it is here it seems fairly easy to extend it to other models so I am less concerned about maintenance.

Looking at ActiveRecord::SQLite::Index code I do see a few hacky things with AR I hope this won't bite us down the road

@marcoroth WDYT ?

Thanks again Kasper for this

test/models/talk_test.rb

kaspth · 2025-01-23T22:45:42Z

@kaspth I tested it locally and it works great. I pushed a small extra test as I was playing with it.

Perfect!

Overall I like how we are getting closer to the ActiveRecord native API. It is clearly code that I wouldn't have been able to write. At first I was a bit concerned about maintenance in the future. but now that it is here it seems fairly easy to extend it to other models so I am less concerned about maintenance.

Yeah, I've definitely been worried about this too. I think overall, the savings we're getting from using more standard Active Record and the hoops we have to jump through to get that are less concerning than the raw SQL hoops we were doing before.

And since that hoop logic is condensed in ActiveRecord::SQLite::Index it can be shared across models. Or even upstreamed into Rails. I've been wondering if @fractaledmind has thoughts on that part.

I'm also planning on doing a write up about how this works for future reference. I'm quite proud of having figured this out really.

Looking at ActiveRecord::SQLite::Index code I do see a few hacky things with AR I hope this won't bite us down the road

For sure! I think attributes_for_create is the hackiest part and that seems pretty stable to me, although it is technically private API.

marcoroth · 2025-01-23T23:47:22Z

Looks good to me!

fractaledmind · 2025-01-24T00:14:39Z

I'll give this a thorough review tomorrow. Interested to dive in tho

kaspth commented Dec 22, 2024

View reviewed changes

adrienpoly mentioned this pull request Jan 2, 2025

add fuzzy search for speakers #547

Merged

kaspth added 5 commits January 20, 2025 00:44

Refactor Talk indexing into a Talk::Index record.

72524bf

This does take some extra work since Active Record can't detect `rowid` is the primary key. But with some awareness around that, it's pretty smooth sailing and we can entirely remove a lot of the custom SQL with standard Active Record.

Fix method name update

ce48d01

Fix constant reference

982a95a

Let's try this

598fdcb

Remove internal to the virtual table attributes; Rails shouldn't touc…

62dd179

…h these

kaspth force-pushed the talk-index-virtual-table-model branch from e702e0c to 62dd179 Compare January 19, 2025 23:45

Fix Active Record not recognizing rowid, so we don't have to do custo…

2310449

…m SQL

kaspth commented Jan 20, 2025

View reviewed changes

We already covered this bit earlier

bae444d

kaspth added a commit to kaspth/rubyvideo that referenced this pull request Jan 21, 2025

Refactor Speaker indexing into a Speaker::Index record.

1c21d37

Follow-up to adrienpoly#540

kaspth mentioned this pull request Jan 21, 2025

Refactor Speaker indexing into a Speaker::Index record. #593

Open

Omit deleting old indexes; skip some queries when reindexing

8b46e6d

kaspth added a commit to kaspth/rubyvideo that referenced this pull request Jan 22, 2025

Refactor Speaker indexing into a Speaker::Index record.

8f01dab

Follow-up to adrienpoly#540

play a bit with the tests

415c127

adrienpoly approved these changes Jan 23, 2025

View reviewed changes

kaspth commented Jan 23, 2025

View reviewed changes

test/models/talk_test.rb Outdated Show resolved Hide resolved

Update test/models/talk_test.rb

8ca8eee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Talk indexing into a Talk::Index record. #540

Refactor Talk indexing into a Talk::Index record. #540

kaspth commented Dec 22, 2024

kaspth Dec 22, 2024

kaspth commented Dec 22, 2024

kaspth commented Dec 22, 2024 •

edited

Loading

kaspth commented Dec 23, 2024

kaspth commented Dec 23, 2024

adrienpoly commented Dec 24, 2024

kaspth commented Jan 4, 2025

kaspth Jan 20, 2025

kaspth Jan 20, 2025

kaspth Jan 20, 2025

kaspth Jan 20, 2025

kaspth Jan 20, 2025 •

edited

Loading

kaspth Jan 20, 2025

adrienpoly Jan 20, 2025

kaspth Jan 21, 2025

kaspth Jan 21, 2025

kaspth commented Jan 20, 2025

adrienpoly commented Jan 22, 2025

adrienpoly left a comment

kaspth commented Jan 23, 2025

marcoroth commented Jan 23, 2025

fractaledmind commented Jan 24, 2025

Refactor Talk indexing into a Talk::Index record. #540

Are you sure you want to change the base?

Refactor Talk indexing into a Talk::Index record. #540

Conversation

kaspth commented Dec 22, 2024

Choose a reason for hiding this comment

kaspth commented Dec 22, 2024

kaspth commented Dec 22, 2024 • edited Loading

kaspth commented Dec 23, 2024

kaspth commented Dec 23, 2024

adrienpoly commented Dec 24, 2024

kaspth commented Jan 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth commented Jan 20, 2025

adrienpoly commented Jan 22, 2025

adrienpoly left a comment

Choose a reason for hiding this comment

kaspth commented Jan 23, 2025

marcoroth commented Jan 23, 2025

fractaledmind commented Jan 24, 2025

kaspth commented Dec 22, 2024 •

edited

Loading

kaspth Jan 20, 2025 •

edited

Loading