Add minimal FTS support #3282
Add minimal FTS support #3282
Conversation
Pull Request Test Coverage Report for Build 4887743905
💛 - Coveralls |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| EnsureIsOpen(); | ||
|
|
||
| var (primitive, handles) = value.ToNative(); | ||
| NativeMethods.string_fts(this, realm, propertyIndex, primitive, out var ex); |
Check notice
Code scanning / CodeQL
Calls to unmanaged code
| @@ -61,6 +61,10 @@ | |||
| public static extern void string_like(QueryHandle queryPtr, SharedRealmHandle realm, IntPtr property_ndx, | |||
| PrimitiveValue primitive, [MarshalAs(UnmanagedType.U1)] bool caseSensitive, out NativeException ex); | |||
|
|
|||
| [DllImport(InteropConfig.DLL_NAME, EntryPoint = "query_string_fts", CallingConvention = CallingConvention.Cdecl)] | |||
| public static extern void string_fts(QueryHandle queryPtr, SharedRealmHandle realm, IntPtr property_ndx, | |||
Check notice
Code scanning / CodeQL
Unmanaged code
Realm/Realm/Attributes/IndexMode.cs
Outdated
| namespace Realms | ||
| { | ||
| #if PRIVATE_INDEXMODE | ||
| internal enum IndexMode |
There was a problem hiding this comment.
Rename to IndexType and set a base type.
| #if TEST_WEAVER | ||
| [Required] | ||
| #endif | ||
| [Indexed(IndexMode.FullText)] |
There was a problem hiding this comment.
Design question: Doesn't this API prevent further customization of Full-Text? I.e I would assume it is very possible that we would want to be able to configure various attributes on Full-Text once/when we increase its capabilities. I.e. this is the full-text annotation in Android: https://developer.android.com/reference/androidx/room/Fts4
There was a problem hiding this comment.
It kinda does, yes. We could add more arguments to the Indexed attribute that relate only to FTS if we wanted to or we can introduce another attribute in the future. I opted against going the another attribute route right now because we don't have short/medium term plans to extend our FTS support and didn't want to bake too many assumptions about the future of the feature into the initial API. It's also the API we agreed on in the Scope, so figured everyone is on board with it.
There was a problem hiding this comment.
The scope mention that the API for other SDKs should be fleshed out in the design doc, which never seemed to happen, so not sure I agree this was the API we wanted to ship with. I read it as just an example of how it could look if we exposed the Core API.
Maybe I'm biased because other frameworks on Android use a dedicated annotation for this, but it seems counterproductive to ship an API that we know for sure we most likely have to break if/when we add more features, even if they are not planned right now. Especially since this API doesn't really provide any meaningful type-safety anyway. Extending the current [Indexed] with more attributes also seems a bit weird as they would be meaningless for the normal index type.
So I would strongly encourage a direction with its own annotation, In Kotlin I exposed it with a @FullText annotation here: https://github.com/realm/realm-kotlin/pull/1368/files#diff-327b30a309e034a752eb56ac993d64f6d291cd9e7af0ddd9018947bf6fddacf2R88
There was a problem hiding this comment.
There's nothing preventing us from shipping a new attribute in the future in a backwards compatible way. I'd rather not prematurely optimize for a case when more configuration options may come in before we know what these will look like and if an attribute would be the best way to go about them - e.g. if we want to allow declaring an FTS index over multiple properties, doing so via attributes could end up being clunky and we may choose a different strategy. So rather than trying to design an API for the future, I'd rather extend the existing one we have that has a meaningful name and is well positioned to support our current needs. If our needs change beyond what is currently supported, we'll figure something out, but as I said, I don't envision this happening in the short/medium term - if ever.
There was a problem hiding this comment.
I wouldn't call this a premature optimization, as semantically you are getting the same API, it is just more flexible. While I agree that an FTS index over multiple properties would be slightly awkward using annotations (but not impossible), most modifications like tokenizer strategy, casing, mixing normal and FTS index would all be supported easily by using a dedicated annotation.
| internal static readonly LazyMethod ContainsStringComparison = Capture<string>(s => QueryMethods.Contains(s, string.Empty, StringComparison.Ordinal)); | ||
|
|
||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| internal static readonly LazyMethod LegacyLike = Capture<string>(s => s.Like(string.Empty, true)); |
Check warning
Code scanning / CodeQL
Call to obsolete method
| @@ -59,9 +60,19 @@ | |||
| { | |||
| internal static readonly LazyMethod Contains = Capture<string>(s => s.Contains(string.Empty)); | |||
|
|
|||
| internal static readonly LazyMethod ContainsStringComparison = Capture<string>(s => s.Contains(string.Empty, StringComparison.Ordinal)); | |||
| #pragma warning disable CS0618 // Type or member is obsolete | |||
| internal static readonly LazyMethod InstanceContainsStringComparison = Capture<string>(s => s.Contains(string.Empty, StringComparison.Ordinal)); | |||
Check warning
Code scanning / CodeQL
Call to obsolete method
Description
Fixes #3281
Blocked on realm/realm-core#6465
TODO