CBL-6651: createIndex doesn't support creating partial index with N1Q… #2214
Conversation
…L language Extended IndexSpec to allow the where clause when the query langugage is N1QL. We put it in IndexSpec::options. In this commit, we only do it for ValueIndex and FTSIndex. With JSON query language, the where clause can be either in the new option or in IndexSpec::expression together with the what clause. With N1QL query language, the where clause has to be in the new option, and IndexSpec::expression contains only the what clause. This commit is to get ready for C4IndexOptions
| @@ -170,8 +170,27 @@ TEST_CASE_METHOD(FTSTest, "Query Full-Text Stop-words In Target", "[Query][FTS]" | |||
| TEST_CASE_METHOD(FTSTest, "Query Full-Text Partial Index", "[Query][FTS]") { | |||
| // the WHERE clause prevents row 4 from being indexed/searched. | |||
| IndexSpec::FTSOptions options{"english", true}; | |||
| store->createIndex("sentence", R"-({"WHAT": [[".sentence"]], "WHERE": [">", ["length()", [".sentence"]], 70]})-", | |||
| IndexSpec::kFullText, options); | |||
| switch ( GENERATE(0, 1, 2) ) { | |||
There was a problem hiding this comment.
It would be simpler/clearer to just have three SECTION blocks. Then Catch will do the logging.
| @@ -45,6 +45,11 @@ namespace litecore { | |||
| bool ignoreDiacritics{}; ///< True to strip diacritical marks/accents from letters | |||
| bool disableStemming{}; ///< Disables stemming | |||
| const char* stopWords{}; ///< NULL for default, or comma-delimited string, or empty | |||
| const char* where; | |||
There was a problem hiding this comment.
Why don't we just add where to IndexSpec itself, since it's supported by multiple index types. Then you don't need the optionWhere method, or the ValueOptions type.
Also, it should be an alloc_slice, not a char*, since the latter has no ownership. (Yes, stopWords is a char*, which was probably a bad idea, but that was a long time ago and IIRC the thought was that it's going to point to a hardcoded string constant?)
There was a problem hiding this comment.
I have noticed it and will make it alloc_slice when I do the C4 API, where "where" will get the value from C4IndexOptions and I cannot rely on C4 client for the storage.
I thought about adding "where" to IndexSpec, but it would a lot of code changes because of the current API,
KeyStore::createIndex(slice name, slice expression, QueryLanguage queryLanguage, IndexSpec::Type type,
IndexSpec::Options options)
I have to change the signature or add an overload.
Perhaps add an overload is easier. Having it in options is somewhat awkward.
| hasWhere = true; | ||
| std::stringstream ss; | ||
| ss << "SELECT " << expression.asString() << " FROM _ WHERE " << optWhere; | ||
| result = (MutableDict*)n1ql::parse(ss.str(), &errPos); |
There was a problem hiding this comment.
On parse error, errPos will be wrong since the string you're parsing isn't what was given to the API. It'll be off by 7 if the error is in expression, and of course a larger variable number if the error is in optWhere.
Also, there's no way for the caller to distinguish whether the error is from the main expression or the 'where' clause ... resolving that might require a change going up to the C API.
There was a problem hiding this comment.
Yes, this error position could be mysterious to the customers. In the error back to the C4,
if ( !result ) { throw Query::parseError("N1QL syntax error in index expression", errPos); }
I am thinking to include the actual expression in the error message. The actual expression may not be exactly what users passed in, but they (and we when we have to debug) find the position in the inputs.
…L language
Extended IndexSpec to allow the where clause when the query langugage is N1QL. We put it in IndexSpec::options. In this commit, we only do it for ValueIndex and FTSIndex.
With JSON query language, the where clause can be either in the new option or in IndexSpec::expression together with the what clause. With N1QL query language, the where clause has to be in the new option, and IndexSpec::expression contains only the what clause.
This commit is to get ready for C4IndexOptions