Making Schema names optional for Table Valued Functions (#981)

* commit for new added test * commit 2 * update * Fixed all the issues related to filtering of data in Virtual Table * Binder Type enhancement * trying to fix BestIndex * Fixed the single column issue in VT * Performance Tests Added * ValueExp_Issue_Id_Change due to conflicts while merging with main * Added flag for binder info to check whether the binder is for a parameter inside InVirtualSet() or IdSet()....useful for BindIdSet() * Tests Added * Crash for following expression stopped by providing null checks SELECT x FROM (with cte(x) as(select ECInstanceId from meta.ECClassDef) select x from cte), test.IdSet('[1,2,3,4,5]') where id = x group by x * Kepping concurrentQueryImpl as close to as it was with minimal changes * schema name changed to ECVLib and also file name changed * cleanup * more cleanup * Performanvce Tests Updated * some comments resolved * Comments regarding constant name of IdSet table resolved * binderInfo refactoring * added flag to call _onbeforefirststep() once in PragmaECSQLStatement and renamed _OnBeforeStep() to _ONBeforeFirstStep() * changes as per suggestions by Affan * Performance test updated * Tests updated to prevent failure in pipeline * tests updated * update in logic in IModelJsNative.cpp and concurrentquery * performance tests indentation updated * final update * OnBeforeFirstStep() logic updated by using m_isFirstStep flag * Commit for schema names optional in table valued functions * More Tests added * issue reporter update * Added flag checking to m_isFirstStep flag so that when actually flag is false we reset it to true and vice versa * logic update * update * update * commiting update * comment added * comment added * Tests added and updated * removing m_isFirstStep and identifying first step using statement state * Comment updated * Fixed the issue with the query SELECT e.i FROM aps.TestElement e INNER JOIN ECVLib.IdSet(?) v ON e.ECInstanceId = v.id * More tests added * More Performance Tests added * indentation issue solved * Update in logic to get statement state * Changelog updated * comments updated * Updates in code * Tests added * ECSql version updated * Colin's comment resolved and appropriate tests added --------- Co-authored-by: affank <[email protected]> (cherry picked from commit ffc5996)
iTwin · Feb 6, 2025 · 801f0d3 · 801f0d3
1 parent 6bd848b
commit 801f0d3
Show file tree

Hide file tree

Showing 14 changed files with 411 additions and 79 deletions.
diff --git a/iModelCore/ECDb/CHANGES.md b/iModelCore/ECDb/CHANGES.md
@@ -5,7 +5,15 @@ This document including important changes to syntax or file format.
 | Module  | Version   |
 | ------- | --------- |
 | Profile | `4.0.0.5` |
-| ECSQL   | `2.0.1.1` |
+| ECSQL   | `2.0.2.0` |
+
+## ## `01/29/2025`: Made schema names optional for table valued functions
+* ECSql version change `2.0.1.1` -> `2.0.2.0`.
+* Made schema names optional for table valued functions
+* Table valued functions will now work with and without schema names. Example :- `SELECT * FROM json_each(:json_param)` & 
+    `SELECT * FROM json1.json_each(:json_param)` both are now valid queries from ECSQL perspective.
+* Example: `Select test.str_prop, test.int_prop, v.id from ts.A test RIGHT OUTER JOIN IdSet(:idSet_param) v on test.ECInstanceId = v.id`,
+           `SELECT * FROM json_each(:json_param)`.
 
 ## ## `01/22/2025`: Added IdSet Virtual Table in ECSQL
 * ECSql version change `2.0.1.0` -> `2.0.1.1`.

diff --git a/iModelCore/ECDb/ECDb/ECSql/ClassRefExp.cpp b/iModelCore/ECDb/ECDb/ECSql/ClassRefExp.cpp
@@ -13,17 +13,31 @@ Exp::FinalizeParseStatus TableValuedFunctionExp::_FinalizeParsing(ECSqlParseCont
     if (FinalizeParseMode::BeforeFinalizingChildren == mode) {
         const auto& vsm = ctx.GetECDb().Schemas().Main().GetVirtualSchemaManager();
         const auto classValuedFunc = GetFunctionExp()->GetFunctionName();
-        const auto tableViewClassP = vsm.GetClass(m_schemaName, classValuedFunc);
+        size_t numberOfClasses = 0; // The numberOfClasses variable gives us the exact number of classes found which is used for more personalized error message
+        const auto tableViewClassP = m_schemaName.EqualsI("") ? vsm.FindClass(classValuedFunc, numberOfClasses) : vsm.GetClass(m_schemaName, classValuedFunc);
         if (tableViewClassP == nullptr) {
-            ctx.Issues().ReportV(
+            if(numberOfClasses == 0)
+            {
+                ctx.Issues().ReportV(
                 IssueSeverity::Error,
                 IssueCategory::BusinessProperties,
                 IssueType::ECDbIssue,
                 ECDbIssueId::ECDb_0451,
-                "TableValuedFunction %s.%s() has no ECClass describing its output.",
-                m_schemaName.c_str(),
-                classValuedFunc.c_str()
-            );
+                "TableValuedFunction %s() has no ECClass describing its output.",
+                m_schemaName.EqualsIAscii("") ? classValuedFunc.c_str() : m_schemaName.append(".").append(classValuedFunc).c_str()
+                );
+            }
+            else if(numberOfClasses > 1)
+            {
+                ctx.Issues().ReportV(
+                IssueSeverity::Error,
+                IssueCategory::BusinessProperties,
+                IssueType::ECDbIssue,
+                ECDbIssueId::ECDb_0738,
+                "TableValuedFunction %s() has more than one ECClass describing its output. Unable to understand ambiguous reference",
+                m_schemaName.EqualsIAscii("") ? classValuedFunc.c_str() : m_schemaName.append(".").append(classValuedFunc).c_str()
+                );
+            }
             return Exp::FinalizeParseStatus::Error;
         }
         m_virtualEntityClass = tableViewClassP->GetEntityClassCP();

diff --git a/iModelCore/ECDb/ECDb/ECSql/ECSqlParser.cpp b/iModelCore/ECDb/ECDb/ECSql/ECSqlParser.cpp
@@ -1608,7 +1608,7 @@ BentleyStatus ECSqlParser::ParseTableRef(std::unique_ptr<ClassRefExp>& exp, OSQL
                 if (SUCCESS == ParseCommonTableBlockName(cteBlockNameExp, *thirdNode)) {
                     rangeClassRef = std::move(cteBlockNameExp);
                 }
-                if (SUCCESS == ParseTableValuedFunction(tableValueFunc, *thirdNode)) {
+                else if (SUCCESS == ParseTableValuedFunction(tableValueFunc, *thirdNode)) {
                     rangeClassRef = std::move(tableValueFunc);
                 }
             }
@@ -2075,10 +2075,10 @@ BentleyStatus ECSqlParser::ParseTableValuedFunction(std::unique_ptr<TableValuedF
     }
 
     const size_t pathLength = pathNode->count();
-    if (pathLength  != 2) {
+    if (pathLength != 1 && pathLength  != 2) 
         return ERROR;
-    }
 
+    if(pathLength == 2){
     auto schemaNode = pathNode->getChild(0);
     auto functionNode = pathNode->getChild(1);
 
@@ -2098,6 +2098,27 @@ BentleyStatus ECSqlParser::ParseTableValuedFunction(std::unique_ptr<TableValuedF
         }
 
     exp = std::make_unique<TableValuedFunctionExp>(schemaName.c_str(), std::move(memberFuncCall), PolymorphicInfo::NotSpecified());
+    }
+    else if(pathLength == 1)
+    {
+        auto functionNode = pathNode->getChild(0);
+
+        if(functionNode == nullptr) return ERROR;
+
+        if (functionNode->getChild(1) == nullptr || functionNode->getChild(1)->isLeaf()) { // We also need to check the leaf condition here because functionNode->getChild(1) is the argument node and that node should have children
+            return ERROR;
+        }
+
+        std::unique_ptr<MemberFunctionCallExp> memberFuncCall;
+        if (functionNode != nullptr)
+            {
+            if (SUCCESS != ParseMemberFunctionCall(memberFuncCall, *functionNode, true))
+                return ERROR;
+            }
+
+        exp = std::make_unique<TableValuedFunctionExp>("", std::move(memberFuncCall), PolymorphicInfo::NotSpecified());
+    }
+
     return SUCCESS;
 }
 
@@ -2244,16 +2265,19 @@ BentleyStatus ECSqlParser::ParseMemberFunctionCall(std::unique_ptr<MemberFunctio
         return ERROR;
         }
 
-    BeAssert(parseNode.count() == 2);
+    if(parseNode.count() != 2)
+        return ERROR;
     OSQLParseNode const* argsNode = parseNode.getChild(1);
-    BeAssert(argsNode != nullptr);
+    if(argsNode == nullptr)
+        return ERROR;
     if (argsNode->isLeaf())
         {
         BeAssert(false && "ParseNode passed to ParseMemberFunctionCall is expected to have a non-empty second child node");
         return ERROR;
         }
 
-    BeAssert(argsNode->count() == 3);
+    if(argsNode->count() != 3)
+        return ERROR;
 
     Utf8StringCR functionName = parseNode.getChild(0)->getTokenValue();
     memberFunCallExp = std::make_unique<MemberFunctionCallExp>(functionName, tableValuedFunc);

diff --git a/iModelCore/ECDb/ECDb/IssueReporter.cpp b/iModelCore/ECDb/ECDb/IssueReporter.cpp
@@ -734,6 +734,7 @@ IssueId ECDbIssueId::ECDb_0734 = IssueId("ECDb_0734");
 IssueId ECDbIssueId::ECDb_0735 = IssueId("ECDb_0735");
 IssueId ECDbIssueId::ECDb_0736 = IssueId("ECDb_0736");
 IssueId ECDbIssueId::ECDb_0737 = IssueId("ECDb_0737");
+IssueId ECDbIssueId::ECDb_0738 = IssueId("ECDb_0738");
 
 //---------------------------------------------------------------------------------------
 // @bsimethod

diff --git a/iModelCore/ECDb/ECDb/IssueReporter.h b/iModelCore/ECDb/ECDb/IssueReporter.h
@@ -755,6 +755,7 @@ struct ECDB_EXPORT ECDbIssueId
     static ECN::IssueId ECDb_0735;
     static ECN::IssueId ECDb_0736;
     static ECN::IssueId ECDb_0737;
+    static ECN::IssueId ECDb_0738;
     };
 
 //---------------------------------------------------------------------------------------

diff --git a/iModelCore/ECDb/ECDb/SchemaManagerDispatcher.cpp b/iModelCore/ECDb/ECDb/SchemaManagerDispatcher.cpp
@@ -196,6 +196,27 @@ ECClassCP VirtualSchemaManager::GetClass(Utf8StringCR schemaName, Utf8StringCR c
     return schema->GetClassCP(className.c_str());
 }
 
+/*---------------------------------------------------------------------------------------
+* @bsimethod
++---------------+---------------+---------------+---------------+---------------+------*/
+ECClassCP VirtualSchemaManager::FindClass(Utf8StringCR className, size_t& numberOfClasses) const{
+    BeMutexHolder lock(m_ecdb.GetImpl().GetMutex());
+    std::vector<ECClassCP> v;
+    for(auto it = m_schemas.begin(); it != m_schemas.end(); ++it)
+    {
+        ECClassCP tempClass = GetClass(it->first, className);
+        if(tempClass != nullptr)
+            v.push_back(tempClass);
+    }
+    numberOfClasses = v.size();
+    if(v.size() == 0)
+        return nullptr;
+    else if(v.size() == 1)
+        return v[0];
+    else
+        return nullptr;
+}
+
 /*---------------------------------------------------------------------------------------
 * @bsimethod
 +---------------+---------------+---------------+---------------+---------------+------*/

diff --git a/iModelCore/ECDb/ECDb/SchemaManagerDispatcher.h b/iModelCore/ECDb/ECDb/SchemaManagerDispatcher.h
@@ -119,6 +119,8 @@ struct VirtualSchemaManager : ECN::IECSchemaLocater {
         bool IsValidVirtualSchema(ECN::ECSchemaR schema, Utf8StringR err) const;
         ECN::ECSchemaCP GetSchema(Utf8StringCR schemaName) const;
         ECN::ECClassCP GetClass(Utf8StringCR schemaName, Utf8StringCR className) const;
+        // The numberOfClasses parameter gives us the exact number of classes found which can be used for more personalized error message
+        ECN::ECClassCP FindClass(Utf8StringCR className, size_t& numberOfClasses) const; 
         BentleyStatus Add(Utf8StringCR schemaXml) const;
 };
 //=======================================================================================

diff --git a/iModelCore/ECDb/PublicAPI/ECDb/ECDb.h b/iModelCore/ECDb/PublicAPI/ECDb/ECDb.h
@@ -303,7 +303,7 @@ struct EXPORT_VTABLE_ATTRIBUTE ECDb : Db
     //         e.g. Remove a sql function or change required argument or format of its return value.
     //  Sub1:  Backward compatible change to 'Syntax'. For example adding new syntax/functions but not breaking any existing.
     //  Sub2:  Backward compatible change to 'Runtime'. For example adding a new sql function.
-    static BeVersion GetECSqlVersion() { return BeVersion(2, 0, 1, 1); }
+    static BeVersion GetECSqlVersion() { return BeVersion(2, 0, 2, 0); }
 
     //! Gets the current version of the ECDb profile
     static ProfileVersion CurrentECDbProfileVersion() { return ProfileVersion(4, 0, 0, 5); }

diff --git a/iModelCore/ECDb/Tests/NonPublished/ConcurrentQueryTest_V2.cpp b/iModelCore/ECDb/Tests/NonPublished/ConcurrentQueryTest_V2.cpp
@@ -1163,7 +1163,7 @@ TEST_F(ConcurrentQueryFixture, ReaderBindingForIdSetVirtualTable) {
         idSet.insert(BeInt64Id(40));
         int vsRowCount = 0;
 
-        ECSqlReader  vsReaderIdSet(mgr, "select id from ECVLib.IdSet(?)",
+        ECSqlReader  vsReaderIdSet(mgr, "select id from IdSet(?)",
             ECSqlParams().BindIdSet(1, idSet));
 
         int i = 1;
@@ -1184,7 +1184,7 @@ TEST_F(ConcurrentQueryFixture, ReaderBindingForIdSetVirtualTable) {
         idSet.insert(BeInt64Id(40));
         int vsRowCount = 0;
 
-        ECSqlReader  vsReaderIdSet(mgr, "select ECInstanceId from meta.ECClassDef, ECVLib.IdSet(?) where ECInstanceId = id",
+        ECSqlReader  vsReaderIdSet(mgr, "select ECInstanceId from meta.ECClassDef, IdSet(?) where ECInstanceId = id",
             ECSqlParams().BindIdSet(1, idSet));
 
         int i = 1;
@@ -1200,7 +1200,7 @@ TEST_F(ConcurrentQueryFixture, ReaderBindingForIdSetVirtualTable) {
         auto& mgr = ConcurrentQueryMgr::GetInstance(m_ecdb);
         int vsRowCount = 0;
 
-        ECSqlReader  vsReaderIdSet(mgr, "select ECInstanceId from meta.ECClassDef, ECVLib.IdSet(?) where ECInstanceId = id",
+        ECSqlReader  vsReaderIdSet(mgr, "select ECInstanceId from meta.ECClassDef, IdSet(?) where ECInstanceId = id",
             ECSqlParams().BindId(1, BeInt64Id(33)));
 
         while(vsReaderIdSet.Next())
@@ -1220,7 +1220,7 @@ TEST_F(ConcurrentQueryFixture, ReaderBindingForIdSetVirtualTable) {
         idSet.insert(BeInt64Id(40));
         int vsRowCount = 0;
 
-        ECSqlReader  vsReaderIdSet(mgr, "select id from ECVLib.IdSet(?)",
+        ECSqlReader  vsReaderIdSet(mgr, "select id from IdSet(?)",
             ECSqlParams().BindIdSet(1, idSet));
 
         try{