Add support for the FTS5 trigram tokenizer

[Jnosh]

I had added support for the newish FTS5 trigram tokenizer in my own project at some point. Upstreaming it here after some polish.

• I kept the documentation basic, the trigram tokenizer is a bit different than the regular ones and has some caveats but probably best to leave it to the SQLite documentation to lists the details?

• The tokenizer (currently) has two boolean arguments but only 3 combinations are valid so I implemented the options as a single enum in Swift. I hope that is OK? Feedback welcome, especially on the enum and case naming.

• I tried to add GRDBCUSTOMSQLITE, GRDBCIPHER conditionals and availability annotations as needed. I would welcome if you had a look if they look sensible.

• One of the tokenizer options (remove_diacritics, e.g. TrigramTokenizerMatching.caseInsensitiveRemovingDiacritics) is available from SQLite 3.45 (according to an SQLite forum post, it's not mentioned in the release notes).
  That is newer than what Apple currently ships so this option can't currently be used with the system provided SQLite. I was wondering if you have a preferred way of handling that.
  For now I added an @available(*, unavailable) annotation with a message indicating that a future OS release is required.
  Alternatively the stdlib approach with an @available(macOS 9999, ...) annotation could be used but I feel a custom message is maybe a bit more self-explanatory.
  Finally we could just comment that option out (for the non-custom build) until Apple ships a release with SQLite >3.45.

• I made a minor adjustment in FTS5Tokenizer.swift to create a null-terminated C string when invoking a tokenizer from GRDB to work around a bug with the trigram tokenizer where it doesn't always handle non-null terminated strings correctly.
  The string contents were also copied before so this shouldn't perform worse.

Pull Request Checklist

• CONTRIBUTING: You have read https://github.com/groue/GRDB.swift/blob/master/CONTRIBUTING.md
• BRANCH: This pull request is submitted against the development branch.
• DOCUMENTATION: Inline documentation has been updated.
• DOCUMENTATION: README.md or another dedicated guide has been updated.
• TESTS: Changes are tested.
• TESTS: The make smokeTest terminal command runs without failure.

[groue]

Hello @Jnosh, thank you very much for this pull request. I intended to review this week-end, but I could not. Please stay tuned.

[Jason-Abbott]

Did some minor testing with this since it aligned with testing my own stuff.

I defined an FTS5 virtual table with trigram tokenizer using new code here, loaded 105,000 word manuscript, each paragraph its own row, and ran some MATCHes. That leaves plenty untested but it worked perfectly for me.

[groue]

I had added support for the newish FTS5 trigram tokenizer in my own project at some point. Upstreaming it here after some polish.

🥳 Thank you @Jnosh!

• I kept the documentation basic, the trigram tokenizer is a bit different than the regular ones and has some caveats but probably best to leave it to the SQLite documentation to lists the details?

👍 I like it. In my review I just added a link where some details were missing.

• The tokenizer (currently) has two boolean arguments but only 3 combinations are valid so I implemented the options as a single enum in Swift. I hope that is OK? Feedback welcome, especially on the enum and case naming.

Yes, I have a feedback ;-) See the review below.

• I tried to add GRDBCUSTOMSQLITE, GRDBCIPHER conditionals and availability annotations as needed. I would welcome if you had a look if they look sensible.

#if GRDBCUSTOMSQLITE || GRDBCIPHER is perfect. It translates into a runtime error if trigram is missing, that people will quickly catch it at runtime. On Apple platforms, we can use availability checks 👍.

• One of the tokenizer options (remove_diacritics, e.g. TrigramTokenizerMatching.caseInsensitiveRemovingDiacritics) is available from SQLite 3.45 (according to an SQLite forum post, it's not mentioned in the release notes).
  That is newer than what Apple currently ships so this option can't currently be used with the system provided SQLite. I was wondering if you have a preferred way of handling that.
  For now I added an @available(*, unavailable) annotation with a message indicating that a future OS release is required.

OK. @available(*, unavailable, message: "Requires a future OS release that includes SQLite >=3.45") is perfect. See my review comments for more details.

• I made a minor adjustment in FTS5Tokenizer.swift to create a null-terminated C string when invoking a tokenizer from GRDB to work around a bug with the trigram tokenizer where it doesn't always handle non-null terminated strings correctly.
  The string contents were also copied before so this shouldn't perform worse.

Thanks for modernizing this old code 👍

[groue]

Thank you very much! Please find my comments below.

[groue]

I have read the SQLite Documentation which says:

remove_diacritics [...] It may only be set to 1 if the case_sensitive options is set to 0 - setting both options to 1 is an error.

Still, I would suggest exposing two distinct options for case_sensitive and remove_diacritics.

The reasons for this suggestion are:

1. As time has passed, I have learned that the Swift version of infrequently used SQLite APIs should be as close to the original as possible. It should have the full SQLite smell. This spares the user from translating the SQLite knowledge that was not always easily acquired into a foreign Swift API.

2. SQLite evolves and fixes bugs. Today both options only support 0 and 1, but this may change. It is good to let the user extend the list of options, even if GRDB api lags behind:

   t.tokenizer = trigram(
     caseSensitive: .init(rawValue: 2),
     diacritics: .init(rawValue: 3))

   This kind of SQLite evolution has happened for Unicode61: remove_diacritics=2 was introduced once a bug was discovered in remove_diacritics=1.

Some GRDB APIs, such as FTS5.Diacritics used by Unicode61, do not support such extensibility. This is not an example to follow.

---

Maybe we could aim at:

// Public usage
t.tokenizer = trigram(
  caseSensitive: true, 
  diacritics: .remove)

API:

public struct FTS5TokenizerDescriptor {
    /// The "trigram" tokenizer.
    ///
    /// For example:
    ///
    /// ```swift
    /// try db.create(virtualTable: "book", using: FTS5()) { t in
    ///     t.tokenizer = .trigram()
    /// }
    /// ```
    ///
    /// Related SQLite documentation: <https://sqlite.org/fts5.html#the_trigram_tokenizer>
    ///
    /// - parameters:
    ///     - caseSensitive: Unless specified, performs a case insensitive matching.
    ///     - diacritics: Unless specified, diacritics are not removed before matching.
    public static func trigram(
        caseSensitive: FTS5.TrigramCaseSensitiveOption? = nil,
        diacritics: FTS5.TrigramDiacriticsOption? = nil,
    ) -> FTS5TokenizerDescriptor { ... }
}

extension FTS5 {
    /// Case sensitivity options for the Trigram FTS5 tokenizer.
    /// Matches the raw "case_sensitive" tokenizer argument.
    ///
    /// Related SQLite documentation: <https://www.sqlite.org/fts5.html#the_trigram_tokenizer>
    public struct TrigramCaseSensitiveOption: RawRepresentable {
        var rawValue: Int
    }
    
    extension TrigramCaseSensitiveOption: ExpressibleByBooleanLiteral {
        /// When true, matches the "case_sensitive=1" trigram tokenizer argument.
        /// When false, it is "case_sensitive=0".
        public init(booleanLiteral value: Bool) {
            self = value ? .init(rawValue: 1) : .init(rawValue: 0)
        }
    }
    
    /// Diacritics options for the Trigram FTS5 tokenizer.
    /// Matches the raw "remove_diacritics" tokenizer argument.
    ///
    /// Related SQLite documentation: <https://www.sqlite.org/fts5.html#the_trigram_tokenizer>
    public struct TrigramDiacriticsOption: RawRepresentable {
        var rawValue: Int
        
        /// Do not remove diacritics. This option matches the raw
        /// "remove_diacritics=0" trigram tokenizer argument.
        public static let keep = Self(rawValue: 0)
        
        /// Remove diacritics. This option matches the raw
        /// "remove_diacritics=1" trigram tokenizer argument.
        public static let remove = Self(rawValue: 1)
    }
}

The FTS5.TrigramDiacriticsOption values (.keep and .remove), which require SQLite 3.45, would be unavailable on Apple platforms. FTS5.TrigramDiacriticsOption can remain fully available.

[groue]

Re-reading myself with a critic eye, I stick to my initial arguments about the "SQLite smell", but I really start to wonder if TrigramCaseSensitiveOption and TrigramDiacriticsOption are that useful.

t.tokenizer = trigram()
t.tokenizer = trigram(caseSensitive: 1)
t.tokenizer = trigram(caseSensitive: 0, removeDiacritics: 1)

public struct FTS5TokenizerDescriptor {
    public static func trigram(
        caseSensitive: Int? = nil,
        removeDiacritics: Int? = nil
    ) -> FTS5TokenizerDescriptor { ... }
}

This would pass my review like a breeze :-)

Yes, this would make the Swift FTS5 apis not very consistent. Keeping the old existing apis is very important, because all code that users can put in a migration must basically compile forever (modifying a migration is a cardinal sin). This should not prevent new apis from being shaped by the experience that was acquired over the years.

I leave it up to your good taste 😄

[Jnosh]

Thank you for the detailed feedback!

When using the raw Int? arguments we obviously can't attach any availability information. Do you think it would make sense to note the SQLite >= 3.45 requirement in the documentation for the removeDiacritics parameter? Or better to just leave that to the SQLite documentation entirely since we can't enforce it anyway?

(In theory we could also have two trigram() methods as well, one only taking the caseSensitive parameter and a second one with both parameters with more restrictive availability. But I don't think that makes sense since that approach won't work if SQLite adds more options for the existing parameters so probably better not to try to enforce availability for the parameters at all than try to partially do it.)

[groue]

But I don't think that makes sense since that approach won't work if SQLite adds more options for the existing parameters so probably better not to try to enforce availability for the parameters at all than try to partially do it.

Ah. I see. Yes it wouldn't be great if people could write t.trigram(caseSensitive: 3) in an app that targets iOS 17. OK, I have been a poor guide here.

So are we back to TrigramCaseSensitiveOption and TrigramDiacriticsOption, with built-in values subject to availability checkings, along with raw initializers for people who know what they are doing?

[Jnosh]

I think that makes sense, that makes it possible to add availability checks while allowing the user to use raw values to bypass restrictions if needed.

A couple of minor questions:

• You've used optional arguments to configure the options when creating the tokenizer as opposed to using a non-optional default value matching the SQLite default. Do I understand correctly that the idea here is for GRDB to respect the SQLite default, even if that default were to change in the future?

public static func trigram(
        caseSensitive: FTS5.TrigramCaseSensitiveOption? = nil,
        diacritics: FTS5.TrigramDiacriticsOption? = nil,
    ) -> FTS5TokenizerDescriptor { ... }

• Do we make TrigramDiacriticsOption itself unavailable for non-custom SQLite or only the remove option? I don't think there is a use case for manually using a raw value on OSs where it isn't supported anyway?

• Thoughts on adding Equatable, Hashable and BitwiseCopyable conformances for TrigramCaseSensitiveOption and TrigramDiacriticsOption; I don't imagine these would see much use but probably no downside to adding them either?

[groue]

❤️

[groue]

@Jnosh I'm very slow to reply because I'm quite busy these days. Please be assured I did not forget this PR.

[Jnosh]

No worries!