CBL-6671: Options of FTS partial index is not fully encoded in the SQ… #2220

jianminzhao · 2025-01-30T08:19:01Z

…Lite table when it is registered

Bumped Schema version.
In the new version, added a column, whereClause, to table indexes.
Upgrade the tables in previous version to include the new column
IndexSpec::whereClause is registered in table "indexes" by the new column.
c4index_getOptions will have "where" passed in with C4IndexOptions.
Tests of the above.

cbl-bot · 2025-01-30T17:31:28Z

Code Coverage Results:

Type	Percentage
branches	67.86
functions	79.31
instantiations	35.26
lines	79.33
regions	75.18

pasin

I have a couple minor comments. Apart from that, look good to me.

C/tests/c4QueryTest.cc

pasin · 2025-01-30T18:51:06Z

LiteCore/Query/SQLiteDataFile+Indexes.cc

+                                else if ( whereA )
+                                    same = whereA->toJSONString() == whereB->toJSONString();
+                            }
+                        }


Maybe a bit cleaner if move this to kFullText case itself.

You can compare Values more simply than this:
same = v1 ? v1->isEqual(v2) : !v2;

This is intentional. I think the logic applies both to Vector and FTS, only at this time, I don't want to touch Vector

pasin · 2025-01-30T18:56:47Z

LiteCore/Query/SQLiteDataFile+Indexes.cc

+                        if ( spec.type == IndexSpec::kFullText ) {
+                            if ( same ) {
+                                auto whatA = spec.what(), whatB = existingSpec->what();
+                                if ( !whatA ? whatB != nullptr : !whatB ) same = false;


Could what be practically NULL? I understand that it's a required field.

I think the purpose here is to check the sameness. NULL should be avoided somewhere else.
I tried it, empty expression will cause crash before coming to here.
Do platforms check it?

If LiteCore returns the error for the empty expression, the platform will return or throw an exception to the user for that error.

snej · 2025-01-30T19:59:48Z

LiteCore/Query/IndexSpec.hh

@@ -112,15 +112,19 @@ namespace litecore {
        /** The optional WHERE clause: the condition for a partial index */
        const fleece::impl::Array* where() const;

+        void setWhereClause(string_view whereClause_) const {
+            if ( !whereClause_.empty() ) whereClause = alloc_slice::nullPaddedString(whereClause_);


Doing nothing when the string is empty could cause bugs later on, if someone wants to clear an existing where clause by calling this. Safer to set whereClause to nullslice in that case.

I see. I intend it to be constant, not to be changed after first assigned.. How about throw if current value is not null? Maybe throw is too rough, but most likely the second assignment is not right. Maybe I can add a warning.

@snej I like it to be almost const, not to be changed after the first assignment. What do you prefer in following?
A. throw if assigned value second time
B. log an warning
C. don't try to limit it

It shouldn't be a const method: it changes the visible state of the object.

…Lite table when it is registered 1. Bumped Schema version. 2. In the new version, added a column, whereClause, to table indexes. 3. Upgrade the tables in previous version to include the new column 4. IndexSpec::whereClause is registered in table "indexes" by the new column. 5. c4index_getOptions will have "where" passed in with C4IndexOptions. 6. Tests of the above.

snej · 2025-01-30T23:57:49Z

LiteCore/Query/IndexSpec.hh

@@ -112,15 +112,19 @@ namespace litecore {
        /** The optional WHERE clause: the condition for a partial index */
        const fleece::impl::Array* where() const;

+        void setWhereClause(string_view whereClause_) const {
+            if ( !whereClause_.empty() ) whereClause = alloc_slice::nullPaddedString(whereClause_);


It shouldn't be a const method: it changes the visible state of the object.

snej · 2025-01-30T23:59:13Z

LiteCore/Query/IndexSpec.hh

+        std::string const   name;           ///< Name of index
+        Type const          type;           ///< Type of index
+        alloc_slice const   expression;     ///< The query expression
+        mutable alloc_slice whereClause;    ///< The where clause. If given, expression should be the what clause


Should not be mutable

CBL-6671: Options of FTS partial index is not fully encoded in the SQ… (#2220) CBL-6534: Fix expiration column upgrade (#2195) (#2219) CBL-6626: SQL++ query not return expected results (#2209) CBL-6667: Remove LIMIT restriction in vector search (#2217) CBL-6670: Code coverage script failing on arm64 (#2218) CBL-6576: Implement Partial Index LiteCore API (#2215) CBL-6651: createIndex doesn't support creating partial index with N1Q… (#2214) CBL-6371: Unnest Query run after delete index produces obtuse error message (#2178) CBL-6625: SQL++ query throws error unexpectedly (#2201)

Includes these tickets in release/3.2: CBL-6671: Options of FTS partial index is not fully encoded in the SQ… (#2220) CBL-6534: Fix expiration column upgrade (#2195) (#2219) CBL-6626: SQL++ query not return expected results (#2209) CBL-6667: Remove LIMIT restriction in vector search (#2217) CBL-6670: Code coverage script failing on arm64 (#2218) CBL-6576: Implement Partial Index LiteCore API (#2215) CBL-6651: createIndex doesn't support creating partial index with N1Q… (#2214) CBL-6371: Unnest Query run after delete index produces obtuse error message (#2178) CBL-6625: SQL++ query throws error unexpectedly (#2201)

Includes these tickets in release/3.2: CBL-6671: Options of FTS partial index is not fully encoded in the SQ… (#2220) CBL-6534: Fix expiration column upgrade (#2195) (#2219) CBL-6626: SQL++ query not return expected results (#2209) CBL-6667: Remove LIMIT restriction in vector search (#2217) CBL-6670: Code coverage script failing on arm64 (#2218) CBL-6576: Implement Partial Index LiteCore API (#2215) CBL-6651: createIndex doesn't support creating partial index with N1Q… (#2214) CBL-6371: Unnest Query run after delete index produces obtuse error message (#2178) CBL-6625: SQL++ query throws error unexpectedly (#2201) * Update XCode project version to 3.2.2 (#2198) * CBL-6625: SQL++ query throws error unexpectedly (#2201) * CBD-6163: BSL License updates - Couchbase Lite Version 3.2 (#2207) Change-Id: Ibc0efec8cff9eea29d1fbbfb087e931f92e09fa1 * CBL-6371: Unnest Query run after delete index produces obtuse error message (#2178) We enhanced the error by adding to the original message, "This table is referenced by an array index, which may have been deleted." * We also enhanced the error message of FTS index in the similar situation. * CBL-6651: createIndex doesn't support creating partial index with N1Q… (#2214) * CBL-6651: createIndex doesn't support creating partial index with N1QL 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 field of IndexSpec, and IndexSpec::expression contains only the what clause. This commit is to get ready for C4IndexOptions * CBL-6576: Implement Partial Index LiteCore API (#2215) Added field "where" to C4IndexOptions and implemented it. * CBL-6670: Code coverage script failing on arm64 (#2218) * CBL-6667: Remove LIMIT restriction in vector search (#2217) The removal of restriction of "LIMIT > 0" matches that in 4.0 line. * CBL-6626: SQL++ query not return expected results (#2209) * CBL-6534: Fix expiration column upgrade (#2195) (#2219) * CBL-6534: Fix expiration column upgrade (#2195) Co-authored-by: jianminzhao <[email protected]> * CBL-6671: Options of FTS partial index is not fully encoded in the SQ… (#2220) * CBL-6671: Options of FTS partial index is not fully encoded in the SQLite table when it is registered 1. Bumped Schema version. 2. In the new version, added a column, whereClause, to table indexes. 3. Upgrade the tables in previous version to include the new column 4. IndexSpec::whereClause is registered in table "indexes" by the new column. 5. c4index_getOptions will have "where" passed in with C4IndexOptions. 6. Tests of the above. --------- Co-authored-by: Vlad Velicu <[email protected]> Co-authored-by: callumbirks <[email protected]> Co-authored-by: Couchbase Robot <[email protected]>

jianminzhao force-pushed the cbl-6671 branch from b230fab to a083d7e Compare January 30, 2025 17:18

jianminzhao requested review from snej and pasin January 30, 2025 18:24

pasin reviewed Jan 30, 2025

View reviewed changes

snej requested changes Jan 30, 2025

View reviewed changes

jianminzhao requested review from snej and pasin January 30, 2025 22:31

pasin approved these changes Jan 30, 2025

View reviewed changes

jianminzhao force-pushed the cbl-6671 branch from 1bf305a to 6e4896b Compare January 30, 2025 23:53

snej requested changes Jan 30, 2025

View reviewed changes

addressing a review comment.

2ca7aef

jianminzhao requested a review from snej January 31, 2025 00:09

snej approved these changes Jan 31, 2025

View reviewed changes

formatting

25e8488

jianminzhao merged commit 2140bfb into release/3.2 Jan 31, 2025
8 of 9 checks passed

jianminzhao deleted the cbl-6671 branch January 31, 2025 01:18

jianminzhao mentioned this pull request Feb 11, 2025

Squashed commit of the following: #2229

Closed

jianminzhao mentioned this pull request Feb 11, 2025

CBL-6737: Merge release/3.2 to master branch #2231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBL-6671: Options of FTS partial index is not fully encoded in the SQ… #2220

CBL-6671: Options of FTS partial index is not fully encoded in the SQ… #2220

jianminzhao commented Jan 30, 2025

cbl-bot commented Jan 30, 2025 •

edited

Loading

pasin left a comment

pasin Jan 30, 2025 •

edited

Loading

snej Jan 30, 2025

jianminzhao Jan 30, 2025

pasin Jan 30, 2025

jianminzhao Jan 30, 2025

pasin Jan 30, 2025

snej Jan 30, 2025

jianminzhao Jan 30, 2025

jianminzhao Jan 30, 2025

snej Jan 30, 2025

snej Jan 30, 2025

snej Jan 30, 2025

CBL-6671: Options of FTS partial index is not fully encoded in the SQ… #2220

CBL-6671: Options of FTS partial index is not fully encoded in the SQ… #2220

Conversation

jianminzhao commented Jan 30, 2025

cbl-bot commented Jan 30, 2025 • edited Loading

pasin left a comment

Choose a reason for hiding this comment

pasin Jan 30, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbl-bot commented Jan 30, 2025 •

edited

Loading

pasin Jan 30, 2025 •

edited

Loading