Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBL-6651: createIndex doesn't support creating partial index with N1Q… #2214

Merged
merged 3 commits into from
Jan 23, 2025
+95 −19
Merged

CBL-6651: createIndex doesn't support creating partial index with N1Q… #2214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
099d822
CBL-6651: createIndex doesn't support creating partial index with N1Q…
jianminzhao Jan 22, 2025
232a553
addressing review feedbacks.
jianminzhao Jan 23, 2025
8c054b2
addressing review comments
jianminzhao Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions LiteCore/Query/IndexSpec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,14 @@ namespace litecore {
switch ( queryLanguage ) {
case QueryLanguage::kJSON:
try {
_doc = Doc::fromJSON(expression);
if ( canPartialIndex() && !whereClause.empty() ) {
std::stringstream ss;
ss << R"({"WHAT": )" << expression.asString() << R"(, "WHERE": )" << whereClause.asString()
<< "}";
_doc = Doc::fromJSON(ss.str());
} else {
_doc = Doc::fromJSON(expression);
}
} catch ( const FleeceException& ) {
error::_throw(error::InvalidQuery, "Invalid JSON in index expression");
}
Expand All @@ -63,17 +70,37 @@ namespace litecore {
int errPos;
alloc_slice json;
if ( !expression.empty() ) {
FLMutableDict result = n1ql::parse(string(expression), &errPos);
if ( !result ) { throw Query::parseError("N1QL syntax error in index expression", errPos); }
json = ((MutableDict*)result)->toJSON(true);
FLMutableDict_Release(result);
MutableDict* result = nullptr;
bool hasWhere = false;
std::stringstream ss;
if ( canPartialIndex() && !whereClause.empty() ) {
hasWhere = true;
ss << "SELECT ( " << expression.asString() << " ) FROM _ WHERE ( "
<< whereClause.asString() << " )";
result = (MutableDict*)n1ql::parse(ss.str(), &errPos);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable.

I also suggest putting parens around the expression and whereClause when constructing the SELECT statement -- that way syntax errors will be more localized instead of possibly only showing up later on in the N1QL.

} else {
result = (MutableDict*)n1ql::parse(expression.asString(), &errPos);
}
if ( !result ) {
string errExpr = "Invalid N1QL in index expression \"";
if ( ss.peek() != EOF ) errExpr += ss.str();
else
errExpr += expression.asString();
errExpr += "\"";
throw Query::parseError(errExpr.c_str(), errPos);
}
if ( hasWhere ) result->remove("FROM"_sl);
json = result->toJSON(true);
FLMutableDict_Release((FLMutableDict)result);
} else {
// n1ql parser won't compile empty string to empty array. Do it manually.
json = "[]";
json = "[]"; // empty WHAT cannot be followed by WHERE clause.
}
_doc = Doc::fromJSON(json);
} catch ( const std::runtime_error& ) {
error::_throw(error::InvalidQuery, "Invalid N1QL in index expression");
} catch ( const std::runtime_error& exc ) {
if ( dynamic_cast<const Query::parseError*>(&exc) ) throw;
else
error::_throw(error::InvalidQuery, "Invalid N1QL in index expression");
}
break;
}
Expand Down
18 changes: 18 additions & 0 deletions LiteCore/Query/IndexSpec.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ namespace litecore {
kVector, ///< Index of ML vector similarity. Uses IndexSpec::VectorOptions.
};

static bool canPartialIndex(Type type_) { return type_ == kValue || type_ == kFullText; }

bool canPartialIndex() const { return canPartialIndex(type); }

/// Options for a full-text index.
struct FTSOptions {
const char* language{}; ///< NULL or an ISO language code ("en", etc)
Expand Down Expand Up @@ -71,6 +75,19 @@ namespace litecore {
IndexSpec(std::string name_, Type type_, alloc_slice expression_,
QueryLanguage queryLanguage = QueryLanguage::kJSON, Options options_ = {});

/// Constructs an index spec.
/// @param name_ Name of the index (must be unique in its collection.)
/// @param type_ Type of the index.
/// @param expression_ The value(s) to be indexed.
/// @param whereClause_ The where clause for the partial index
/// @param queryLanguage Language used for `expression_`; either JSON or N1QL.
/// @param options_ Options; if given, its type must match the index type.
IndexSpec(std::string name_, Type type_, string_view expression_, string_view whereClause_ = {},
QueryLanguage queryLanguage = QueryLanguage::kJSON, Options options_ = {})
: IndexSpec(name_, type_, alloc_slice(expression_), queryLanguage, options_) {
const_cast<alloc_slice&>(this->whereClause) = whereClause_;
}

IndexSpec(const IndexSpec&) = delete;
IndexSpec(IndexSpec&&);

Expand Down Expand Up @@ -101,6 +118,7 @@ namespace litecore {
std::string const name; ///< Name of index
Type const type; ///< Type of index
alloc_slice const expression; ///< The query expression
alloc_slice const whereClause; ///< The where clause. If given, expression should be the what clause
QueryLanguage queryLanguage; ///< Is expression JSON or N1QL?
Options const options; ///< Options for FTS and vector indexes

Expand Down
22 changes: 20 additions & 2 deletions LiteCore/tests/FTSTest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,26 @@ 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);

SECTION("JSON Index Spec with combinged \"what\" and \"where\"") {
REQUIRE(store->createIndex({"sentence",
IndexSpec::kFullText,
R"-({"WHAT": [[".sentence"]], "WHERE": [">", ["length()", [".sentence"]], 70]})-",
{},
QueryLanguage::kJSON,
options}));
}

SECTION("JSON Index Spec with separate \"what\" and \"where\"") {
REQUIRE(store->createIndex({"sentence", IndexSpec::kFullText, R"-([[".sentence"]])-",
R"-([">", ["length()", [".sentence"]], 70])-", QueryLanguage::kJSON, options}));
}

SECTION("N1QL Index Spec") {
REQUIRE(store->createIndex({"sentence", IndexSpec::kFullText, "sentence", "length(sentence) > 70",
QueryLanguage::kN1QL, options}));
}

testQuery("['SELECT', {'WHERE': ['MATCH()', 'sentence', 'search'],\
ORDER_BY: [['DESC', ['rank()', 'sentence']]],\
WHAT: [['.sentence']]}]",
Expand Down
7 changes: 7 additions & 0 deletions LiteCore/tests/LiteCoreTest.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,11 @@ class DataFileTestFixture
alloc_slice blobAccessor(const fleece::impl::Dict*) const override;

string _databaseName{"db"};

protected:
static void logSection(const string& name, int level = 0) {
size_t numSpaces = 8 + level * 4;
std::string spaces(numSpaces, ' ');
fprintf(stderr, "%s--- %s\n", spaces.c_str(), name.c_str());
}
};
18 changes: 15 additions & 3 deletions LiteCore/tests/QueryTest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,14 +562,26 @@ TEST_CASE_METHOD(QueryTest, "Create Partial Index", "[Query]") {
addNumberedDocs(1, 100);
addArrayDocs(101, 100);

store->createIndex("nums"_sl, R"({"WHAT":[[".num"]], "WHERE":["=",[".type"],"number"]})"_sl);

auto [queryJson, expectOptimized] =
GENERATE(pair<const char*, bool>{"['AND', ['=', ['.type'], 'number'], "
"['>=', ['.', 'num'], 30], ['<=', ['.', 'num'], 40]]",
true},
pair<const char*, bool>{"['AND', ['>=', ['.', 'num'], 30], ['<=', ['.', 'num'], 40]]", false});
logSection(string("Query: ") + queryJson);

SECTION("JSON Index Spec with combined \"what\" and \"where\"") {
REQUIRE(store->createIndex("nums"_sl, R"({"WHAT":[[".num"]], "WHERE":["=",[".type"],"number"]})"_sl));
}

SECTION("JSON Index Spec with separate \"what\" and \"where\"") {
REQUIRE(store->createIndex(
{"nums", IndexSpec::kValue, R"([[".num"]])", R"(["=",[".type"],"number"])", QueryLanguage::kJSON}));
}

SECTION("N1QL Index Spec") {
REQUIRE(store->createIndex({"nums", IndexSpec::kValue, "num", "type = 'number'", QueryLanguage::kN1QL}));
}

logSection(string("Query: ") + queryJson + (expectOptimized ? ", " : ", not ") + "optimized");
Retained<Query> query = store->compileQuery(json5(queryJson));
checkOptimized(query, expectOptimized);

Expand Down
6 changes: 0 additions & 6 deletions LiteCore/tests/QueryTest.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,6 @@ class QueryTest : public DataFileTestFixture {
}
}

static void logSection(const string& name, int level = 0) {
size_t numSpaces = 8 + level * 4;
std::string spaces(numSpaces, ' ');
fprintf(stderr, "%s--- %s\n", spaces.c_str(), name.c_str());
}

static string numberString(int n) {
static const char* kDigit[10] = {"zero", "one", "two", "three", "four",
"five", "six", "seven", "eight", "nine"};
Expand Down
Loading