CBL-6400: Port the changes to QueryParser in 3.2 branch (to support U… #2186

jianminzhao · 2024-11-15T22:18:55Z

…NNEST) to 4.0 (master branch)

cbl-bot · 2024-11-15T22:51:10Z

Code Coverage Results:

Type	Percentage
branches	66.33
functions	78.53
instantiations	70.71
lines	77.66
regions	73.51

…NNEST) to 4.0 (master branch)

snej

I haven't reviewed it all yet, but there are some changes to make.

snej · 2024-11-16T00:47:50Z

LiteCore/Query/IndexSpec.cc

+                for ( size_t pos = 0; pos != string::npos && pos < unnestPath.length(); ) {
+                    size_t next = unnestPath.find(KeyStore::kUnnestLevelSeparator, pos);
+                    if ( next == string::npos ) {
+                        splitPaths.push_back(unnestPath.substr(pos));
+                        pos = string::npos;
+                    } else {
+                        splitPaths.push_back(unnestPath.substr(pos, next - pos));
+                        pos = next + KeyStore::kUnnestLevelSeparator.size;
+                    }
+                }


I think you can replace this loop with a call to split from StringUtils.

snej · 2024-11-16T00:51:52Z

LiteCore/Query/IndexSpec.cc

+                string n1qlUnnestPaths = string(splitPaths[0]);
+                for ( unsigned i = 1; i < splitPaths.size(); ++i ) {
+                    n1qlUnnestPaths += ", ";
+                    n1qlUnnestPaths += splitPaths[i];
+                }


So, you're splitting unnestPath with one delimiter, and then joining the pieces with a different delimiter. I think you could do this more simply by just replacing kUnnestLevelSeparator with ", " in unnestPath...?

snej · 2024-11-16T00:53:29Z

LiteCore/Query/IndexSpec.hh


-        mutable FLDoc _doc = nullptr;
+        mutable FLDoc _doc       = nullptr;
+        mutable FLDoc _unnestDoc = nullptr;


This needs to be released in the destructor.

snej · 2024-11-16T00:56:20Z

LiteCore/Query/SQLiteKeyStore+ArrayIndexes.cc

+        if ( currSpec ) {
+            // If there is already index with the index name,
+            // eiher delete the current one, or use it (return false)
+            while ( true ) {


This isn't a loop. I guess you used a while so you could exit it with break? That makes the code confusing IMHO; it'd be clearer just to use ifs.

changed to if's

snej · 2024-11-16T00:56:59Z

LiteCore/Query/SQLiteKeyStore+ArrayIndexes.cc

+            db().deleteIndex(*currSpec);
+        }
+
+        // the following will throw if !spec.arrayOptions() || !spec.arrayOpeionts()->unnestPath


Typo: "arrayOpeionts"

snej · 2024-11-16T00:58:15Z

LiteCore/Query/SQLiteKeyStore+ArrayIndexes.cc

+        }
+
+        // the following will throw if !spec.arrayOptions() || !spec.arrayOpeionts()->unnestPath
+        Array::iterator itPath((const Array*)spec.unnestPaths());


Move this into the first clause of the for statement

snej · 2024-11-16T01:01:50Z

LiteCore/Query/SQLiteKeyStore+Indexes.cc

-        QueryTranslator qp(db(), "", sourceTableName);
-        qp.writeCreateIndex(spec.name, sourceTableName, (FLArrayIterator&)expressions, spec.where(),
-                            (spec.type != IndexSpec::kValue));
+        string          hexName{spec.type == IndexSpec::kArray ? litecore::hexName(sourceTableName) : ""};


Why not use sourceTableName as the alternate value instead of ""? Then you don't have to have conditionals in the following two lines.

changed to use optional.

snej · 2024-11-16T01:03:18Z

LiteCore/Query/Translator/Node.hh

+        Collation                                collation;                 // Current collation in effect
+        bool                                     collationApplied = true;   // False if no COLLATE node generated
+        bool                                     hasGroupBy       = false;  // Current SELECT include GROUP_BY
+        std::unordered_map<string, string> hashedTables;  // hexName(tableName) -> tableName. Used for Unnest tables.


Put this above the bools

snej · 2024-11-16T01:09:07Z

LiteCore/Query/Translator/Node.hh

+        SourceNode* C4NULLABLE                   from{};                    // The main source
+        Collation                                collation;                 // Current collation in effect
+        bool                                     collationApplied = true;   // False if no COLLATE node generated
+        bool                                     hasGroupBy       = false;  // Current SELECT include GROUP_BY


Putting the flag here only works for the top-level query. If there's a nested SELECT expression, the flag will be set in the nested context and the QueryTranslator won't see it.
Instead, the node whose SQL generation depends on hasGroupBy should make that an instance variable, and set it when the node is constructed by looking at its parent SELECT statement.

yes. good point. Passed hasGroupBy from ctx to the UnnestNode.

snej · 2024-11-19T17:48:32Z

LiteCore/Query/SQLiteKeyStore+Indexes.cc

+        std::optional<string> hexName;
+        if ( spec.type == IndexSpec::kArray ) hexName.emplace(litecore::hexName(sourceTableName));
+        QueryTranslator qp(db(), "", hexName.value_or(sourceTableName));
+        qp.writeCreateIndex(spec.name, hexName.value_or(sourceTableName), (FLArrayIterator&)expressions, spec.where(),


What I meant was that it can be simplified to this:

Suggested change

std::optional<string> hexName;

if ( spec.type == IndexSpec::kArray ) hexName.emplace(litecore::hexName(sourceTableName));

QueryTranslator qp(db(), "", hexName.value_or(sourceTableName));

qp.writeCreateIndex(spec.name, hexName.value_or(sourceTableName), (FLArrayIterator&)expressions, spec.where(),

string name = ( spec.type == IndexSpec::kArray ) ? hexName(sourceTableName) : sourceTableName;

QueryTranslator qp(db(), "", name);

qp.writeCreateIndex(spec.name, name, (FLArrayIterator&)expressions, spec.where(),

snej · 2024-11-19T17:51:44Z

LiteCore/Query/Translator/ExprNodes.hh

@@ -124,6 +124,8 @@ namespace litecore::qt {

        void writeSQL(SQLWriter&, slice sqliteFnName, ExprNode* C4NULLABLE param) const;

+        void hasGroupBy() { _hasGroupBy = true; }


The name hasGroupBy is confusing because it sounds like it's an accessor, not a setter.
But this method isn't needed anyway...

snej · 2024-11-19T17:53:10Z

LiteCore/Query/Translator/Node.hh

+        Collation                                collation;  // Current collation in effect
+        std::unordered_map<string, string> hashedTables;     // hexName(tableName) -> tableName. Used for Unnest tables.
+        bool                               collationApplied = true;   // False if no COLLATE node generated
+        bool                               hasGroupBy       = false;  // Current SELECT include GROUP_BY


hasGroupBy isn't needed anymore

snej · 2024-11-19T17:53:52Z

LiteCore/Query/Translator/QueryTranslator.cc

@@ -60,6 +61,8 @@ namespace litecore {
            } else if ( auto p = dynamic_cast<ParameterNode*>(&node) ) {
                // Capture the parameter names:
                _parameters.emplace(p->name());
+            } else if ( auto prop = dynamic_cast<PropertyNode*>(&node) ) {
+                if ( ctx.hasGroupBy ) prop->hasGroupBy();


No, this isn't what I meant. The _hasGroupBy flag should be set by the PropertyNode's constructor itself.

But at the time PropertyNode is constructed, we haven't parsed GROUP_BY. PropertyNode is constructed when source->parseChildExprs(ctx) is executed.

The SourceNode doesn't have to have parsed GROUP_BY yet, it just needs to check whether it exists. Add a flag SelectNode::_hasGroupBy that's set early in the parse method. PropertyNode can check that flag via an accessor.

Got it. Done.

snej · 2024-11-19T17:57:24Z

LiteCore/Query/Translator/Node.hh

+        std::vector<SourceNode*>                 sources;    // All sources
+        SourceNode* C4NULLABLE                   from{};     // The main source
+        Collation                                collation;  // Current collation in effect
+        std::unordered_map<string, string> hashedTables;     // hexName(tableName) -> tableName. Used for Unnest tables.


The only thing that useshashedTables is one method of QueryTranslator; it doesn't belong here. It should be a member of QueryTranslator.

Good point. Done

snej · 2024-11-19T18:05:06Z

LiteCore/Query/Translator/SelectNodes.cc

@@ -362,6 +378,7 @@ namespace litecore::qt {
                    }
                    addChild(_groupBy, group);
                }
+                ctx.hasGroupBy = true;


I don't think this flag is a good idea. The context already has a reference to the SelectNode, so just add a bool hasGroupBy() accessor to SelectNode.

changed as suggested

jianminzhao · 2024-11-21T23:41:25Z

The two failed tests with Jenkins check all passed all tests but timed out afterwards (for some technical reason.)

- CBL-6400: Port the changes to QueryParser in 3.2 branch (to support UNNEST) (#2186) - Update the fleece submodule to the tip of its master branch. (#2189) - CBL-6422: Some tests of VectorSearch are failing in 4.0

CBL-6400: Port the changes to QueryParser in 3.2 branch (to support U…

a1bd20c

…NNEST) to 4.0 (master branch)

jianminzhao force-pushed the cbl-6400 branch from f82a8ad to a1bd20c Compare November 15, 2024 23:27

jianminzhao requested a review from snej November 16, 2024 00:22

snej requested changes Nov 16, 2024

View reviewed changes

addressing review comments.

71da1ec

jianminzhao requested a review from snej November 18, 2024 18:09

snej requested changes Nov 19, 2024

View reviewed changes

reorganized a bit based on review comments.

649a232

jianminzhao force-pushed the cbl-6400 branch from e17d975 to 649a232 Compare November 21, 2024 20:23

jianminzhao requested a review from snej November 21, 2024 23:34

snej approved these changes Nov 21, 2024

View reviewed changes

jianminzhao merged commit c3ecb46 into master Nov 22, 2024
7 of 8 checks passed

jianminzhao deleted the cbl-6400 branch November 22, 2024 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBL-6400: Port the changes to QueryParser in 3.2 branch (to support U… #2186

CBL-6400: Port the changes to QueryParser in 3.2 branch (to support U… #2186

jianminzhao commented Nov 15, 2024

cbl-bot commented Nov 15, 2024 •

edited

Loading

snej left a comment

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 16, 2024

jianminzhao Nov 18, 2024

snej Nov 19, 2024

jianminzhao Nov 19, 2024

snej Nov 19, 2024

snej Nov 19, 2024

snej Nov 19, 2024

jianminzhao Nov 19, 2024

snej Nov 19, 2024

jianminzhao Nov 20, 2024

snej Nov 19, 2024

jianminzhao Nov 19, 2024

snej Nov 19, 2024

jianminzhao Nov 21, 2024

jianminzhao commented Nov 21, 2024

		@@ -124,6 +124,8 @@ namespace litecore::qt {

		void writeSQL(SQLWriter&, slice sqliteFnName, ExprNode* C4NULLABLE param) const;

		void hasGroupBy() { _hasGroupBy = true; }

CBL-6400: Port the changes to QueryParser in 3.2 branch (to support U… #2186

CBL-6400: Port the changes to QueryParser in 3.2 branch (to support U… #2186

Conversation

jianminzhao commented Nov 15, 2024

cbl-bot commented Nov 15, 2024 • edited Loading

snej left a 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

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

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

jianminzhao commented Nov 21, 2024

cbl-bot commented Nov 15, 2024 •

edited

Loading