Added faster alternative to InVirtualSet() #874
Conversation
# Conflicts: # iModelCore/ECDb/ECDb/IssueReporter.cpp # iModelCore/ECDb/ECDb/IssueReporter.h
soham-bentley
requested review from
khanaffan,
pmconne,
calebmshafer,
rschili and
ColinKerr
as code owners
…eter inside InVirtualSet() or IdSet()....useful for BindIdSet()
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
|
/azp run imodel-native |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run imodel-native |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
This pull request is now in conflicts. Could you fix it @soham-bentley? 🙏 |
|
/azp run imodel-native |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| //======================================================================================= | ||
| enum StatementState | ||
| { | ||
| // The first four values are in accordance with sqlite3.c |
There was a problem hiding this comment.
Is this enum structure and these associated comments okay?
There was a problem hiding this comment.
Phew, this is uncharted terrain for me, but I see the native values for VDBE*State are only the first four of these. NOT_PREPARED is not part of the values.
Given that they may add additional values, and the next one would be assigned = 4, this seems dangerous.
It seems much more robust to me to only expose the states which they use and always call "IsPrepared()" before that. That seems to be the intended design.
Your abstraction makes it more convenient to use, but in this case, modifying the API as little as possible seems better.
| } | ||
| { | ||
| ECSqlStatement stmt; | ||
| ASSERT_EQ(ECSqlStatus::Success, stmt.Prepare(m_ecdb, "SELECT ECClassId FROM ECVLib.IdSet('[1,2,3,4,5]'), meta.ECClassDef where ECClassId = id group by ECClassId")); |
There was a problem hiding this comment.
These are the query plan tests. How do these query plans look? @khanaffan
…R JOIN ECVLib.IdSet(?) v ON e.ECInstanceId = v.id
| @@ -684,7 +684,7 @@ void ECSqlExpPreparer::RemovePropertyRefs(ECSqlPrepareContext& ctx, ClassRefExp | |||
| if (propertyNameExp->IsPropertyRef()) | |||
| continue; | |||
| if (propertyNameExp->IsVirtualProperty()) | |||
| break; | |||
There was a problem hiding this comment.
This "break" statement was stopping properties to be added and causing a malfunction in case of JOINS with virtual tables
Is there any specific reason why this was "break" and not "continue" ? @khanaffan @ColinKerr @rschili
There was a problem hiding this comment.
i do not think its a feature, its a bug :)
| DbResult IdSetModule::IdSetTable::IdSetCursor::FilterJSONBasedOnType(BeJsConst& val) { | ||
| if(val.isNull()) | ||
| { | ||
| return BE_SQLITE_ERROR; | ||
| } | ||
| else if(val.isNumeric()) | ||
| { | ||
| uint64_t id = val.GetUInt64(); | ||
| if(id == 0) | ||
| return BE_SQLITE_ERROR; | ||
| m_idSet.insert(id); | ||
| } | ||
| else if(val.isString()) | ||
| { | ||
| uint64_t id; | ||
| BentleyStatus status = BeStringUtilities::ParseUInt64(id, val.ToUtf8CP()); | ||
| if(status != BentleyStatus::SUCCESS) | ||
| return BE_SQLITE_ERROR; | ||
| if(id == 0) | ||
| return BE_SQLITE_ERROR; | ||
| m_idSet.insert(id); | ||
| } | ||
| else | ||
| return BE_SQLITE_ERROR; | ||
| return BE_SQLITE_OK; | ||
| } | ||
|
|
||
| //--------------------------------------------------------------------------------------- |
There was a problem hiding this comment.
This PR has inconsistent bracket style. Even within one method you are switching style. Please use a consistent style, the style used by the rest of the C++ code is preferred
|
Have we gotten feedback on this new syntax? If not I suggest that we put this behind the experimental flag so we can release and get feedback before we lock in on it |
itwinjs-core: iTwin/itwinjs-core#7260
Fixes https://github.com/iTwin/itwinjs-backlog/issues/754