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.

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

Added faster alternative to InVirtualSet() (backport #874) [release/4.11.x] #987

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Feb 4, 2025

itwinjs-core: iTwin/itwinjs-core#7648
Fixes https://github.com/iTwin/itwinjs-backlog/issues/754


This is an automatic backport of pull request #874 done by Mergify.

* 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

* More Tests added

* Added flag checking to m_isFirstStep flag so that when actually flag is false we reset it to true and vice versa

* 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

* Using EqualsI instead of EqualsIAscii

* Comments resolved and tests added

* Made IdSet experimental feature

---------

Co-authored-by: affank <[email protected]>
(cherry picked from commit 001246a)

# Conflicts:
#	iModelJsNodeAddon/api_package/ts/package-lock.json
Copy link
Author

mergify bot commented Feb 4, 2025

Cherry-pick of 001246a has failed:

On branch mergify/bp/release/4.11.x/pr-874
Your branch is up to date with 'origin/release/4.11.x'.

You are currently cherry-picking commit 001246a9.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   iModelCore/BeSQLite/BeSQLite.cpp
	modified:   iModelCore/BeSQLite/PublicAPI/BeSQLite/BeSQLite.h
	modified:   iModelCore/BeSQLite/SQLite/bentley-sqlite.c
	modified:   iModelCore/ECDb/CHANGES.md
	modified:   iModelCore/ECDb/ECDb/BuiltInVTabs.cpp
	modified:   iModelCore/ECDb/ECDb/BuiltInVTabs.h
	modified:   iModelCore/ECDb/ECDb/ConcurrentQueryManagerImpl.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/ArrayECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/ArrayECSqlBinder.h
	modified:   iModelCore/ECDb/ECDb/ECSql/ClassRefExp.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/ClassRefExp.h
	modified:   iModelCore/ECDb/ECDb/ECSql/ECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/ECSqlBinder.h
	modified:   iModelCore/ECDb/ECDb/ECSql/ECSqlPreparedStatement.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/ECSqlPreparedStatement.h
	modified:   iModelCore/ECDb/ECDb/ECSql/ECSqlPreparer.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/ECSqlStatementNoopImpls.h
	modified:   iModelCore/ECDb/ECDb/ECSql/IECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/IdECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/IdECSqlBinder.h
	modified:   iModelCore/ECDb/ECDb/ECSql/NavigationPropertyECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/NavigationPropertyECSqlBinder.h
	modified:   iModelCore/ECDb/ECDb/ECSql/PointECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/PointECSqlBinder.h
	modified:   iModelCore/ECDb/ECDb/ECSql/PragmaECSqlPreparedStatement.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/PragmaECSqlPreparedStatement.h
	modified:   iModelCore/ECDb/ECDb/ECSql/PrimitiveECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/PrimitiveECSqlBinder.h
	modified:   iModelCore/ECDb/ECDb/ECSql/StructECSqlBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/StructECSqlBinder.h
	modified:   iModelCore/ECDb/ECDb/ECSql/ValueExp.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/ValueExp.h
	modified:   iModelCore/ECDb/ECDb/ECSql/VirtualSetBinder.cpp
	modified:   iModelCore/ECDb/ECDb/ECSql/VirtualSetBinder.h
	modified:   iModelCore/ECDb/ECDb/IssueReporter.cpp
	modified:   iModelCore/ECDb/ECDb/IssueReporter.h
	modified:   iModelCore/ECDb/PublicAPI/ECDb/ECDb.h
	modified:   iModelCore/ECDb/PublicAPI/ECDb/ECSqlStatement.h
	modified:   iModelCore/ECDb/PublicAPI/ECDb/IECSqlBinder.h
	modified:   iModelCore/ECDb/Tests/NonPublished/ConcurrentQueryTest_V2.cpp
	new file:   iModelCore/ECDb/Tests/NonPublished/ECDbIdSetVirtualTableTests.cpp
	modified:   iModelCore/ECDb/Tests/NonPublished/ECDbTests.cpp
	new file:   iModelCore/ECDb/Tests/Performance/PerformanceIdSetTests.cpp
	modified:   iModelJsNodeAddon/IModelJsNative.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   iModelJsNodeAddon/api_package/ts/package-lock.json

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@soham-bentley
Copy link
Contributor

/azp run imodel-native

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DanRod1999
Copy link
Contributor

/azp run imodel-native

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@soham-bentley
Copy link
Contributor

/azp run imodel-native

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pmconne pmconne merged commit 9878f37 into release/4.11.x Feb 5, 2025
18 checks passed
@pmconne pmconne deleted the mergify/bp/release/4.11.x/pr-874 branch February 5, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants