store: Populate fulltext columns when copying a subgraph#6687
Merged
Conversation
incrypto32
reviewed
Jul 17, 2026
| // would be silently dropped from the copy (they are nullable, so | ||
| // the error branch below is not taken) and the destination's | ||
| // search index would be left empty. | ||
| if let Some(scol) = src.column_including_fulltext(&dcol.name) { |
Member
There was a problem hiding this comment.
Should can_copy_from also use column_including_fulltext it uses column() currently?
| // the error branch below is not taken) and the destination's | ||
| // search index would be left empty. | ||
| if let Some(scol) = src.column_including_fulltext(&dcol.name) { | ||
| if let Some(msg) = dcol.is_assignable_from(scol, &src.object) { |
Member
There was a problem hiding this comment.
One thing that claude flagged during the review:
is_assignable_fromonly compares FulltextConfig, which is language and algorithm, not fulltext_fields. So a graft that adds a field to include: copies the old tsvectors and searches on the new field quietly return nothing. Comparefulltext_fieldshere too?
incrypto32
approved these changes
Jul 17, 2026
incrypto32
left a comment
incrypto32
left a comment
Member
There was a problem hiding this comment.
Apart from the comments, LGTM
CopyEntityBatchQuery::new looked up source columns via Table::column, which deliberately hides tsvector (fulltext) columns. Because fulltext columns are nullable, they were silently dropped from the copy instead of hitting the non-nullable error path, so the destination's fulltext search column and its GIN index were left empty after a copy/graft. Add Table::column_including_fulltext and use it in the copy so fulltext columns are matched and their already-computed tsvector value is copied verbatim; document that Table::column skips fulltext search columns. Add a regression test that copies the User table and verifies a fulltext query against the copy returns the expected entity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CopyEntityBatchQuery::newlooked up source columns viaTable::column, which deliberately hidestsvector(fulltext) columns. Because fulltext columns are nullable, they were silently dropped from the copy instead of hitting the non-nullable error path, so the destination's fulltext search column and its GIN index were left empty after a copy/graft.Add
Table::column_including_fulltextand use it in the copy so fulltext columns are matched and their already-computed tsvector value is copied verbatim; document thatTable::columnskips fulltext search columns. Add a regression test that copies theUsertable and verifies a fulltext query against the copy returns the expected entity.