Skip to content

Adding support for multi row batched return for ChangesetReader typescript api#1474

Open
soham-bentley wants to merge 14 commits into
mainfrom
soham/adding_multi-row_return_for_typescript_caching
Open

Adding support for multi row batched return for ChangesetReader typescript api#1474
soham-bentley wants to merge 14 commits into
mainfrom
soham/adding_multi-row_return_for_typescript_caching

Conversation

@soham-bentley

@soham-bentley soham-bentley commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Fixes https://github.com/iTwin/itwinjs-backlog/issues/2162
Fixes iTwin/itwinjs-core#9415

itwinjs-core: iTwin/itwinjs-core#9435

Overview

This PR updates the native ChangesetReader plumbing in ECDb and changes the Node addon's TypeScript-facing API to support batched / multi-row retrieval, returning metadata plus old/new stage values per row in a single call. It also refactors the internal design to remove the now-obsolete ChangesetReaderImpl files, introduces an LRU TableColumnCache, and tightens the public API surface.


Key Changes

Node Addon (N-API / TypeScript API)

  • Removes the per-row step() / getValue() / getChangeMetadata() call pattern.
  • Replaces it with a single step(numOfRows, rowOptions) call that:
    • Steps up to numOfRows rows internally.
    • Returns a ChangesetRowData[] array where each element contains { metadata, oldValues, newValues }.
  • Removes the dedicated getValue and getChangeMetadata N-API method registrations (InstanceMethod entries).
  • Adds two new TypeScript interfaces to NativeLibrary.ts:
    interface ChangesetRowMetadata {
      tableName: string;
      opCode: DbOpcode;
      isIndirectChange: boolean;
      isECTable: boolean;
    }
    interface ChangesetRowData {
      metadata: ChangesetRowMetadata;
      oldValues: ChangesetRowValue | undefined;
      newValues: ChangesetRowValue | undefined;
    }
  • Helper methods BuildRowMetadata and BuildRowValue extracted in C++ for cleaner per-row construction.

ECDb — ChangesetReader (public API)

  • ChangesetReader now owns a std::unique_ptr<PreparedChangesetReader> m_innerReader instead of a raw Impl* m_pimpl.
    • Constructor initialises m_innerReader with std::make_unique<PreparedChangesetReader>().
    • Destructor calls m_innerReader.reset() explicitly.
  • All delegation methods (OpenChangesetFile, OpenChangeGroup, OpenInMemoryChangeset, Close, Step, GetTableName, GetOpcode, GetValue, GetECDb, GetColumnCount, GetInstanceKey, IsECTable, IsIndirectChange, SetTableFilters, SetOpcodeFilters, SetECClassNameFilters, ClearTableFilters, ClearOpcodeFilters, ClearECClassNameFilters, EnableStrictMode, DisableStrictMode) now forward to m_innerReader instead of m_pimpl.
  • GetChangeFetchedPropertyNames signature changed from an out-parameter to a pointer return:
    // Before
    ECDB_EXPORT BentleyStatus GetChangeFetchedPropertyNames(std::vector<Utf8String>& out) const;
    // After
    ECDB_EXPORT std::vector<Utf8String> const* GetChangeFetchedPropertyNames() const;
    Returns nullptr when the reader is not positioned on a valid row.
  • Removed the inner struct Impl forward declaration and the PIMPL comment; public header now forward-declares struct PreparedChangesetReader instead.
  • Open methods now accept ECDbCR ecdb as a parameter directly (previously stored on construction), keeping ChangesetReader itself ECDb-agnostic until Open* is called.

ECDb — PreparedChangesetReader (internal refactor)

  • Constructor changed from explicit PreparedChangesetReader(ECDbCR ecdb) to explicit PreparedChangesetReader() — ECDb reference is now passed at open time rather than construction time, allowing the reader to be constructed without an ECDb.
  • m_ecdb changed from ECDbCR (reference) to ECDb const* (pointer, nullptr when closed).
  • Three new internal helper types introduced in PreparedChangesetReader.h:
    • TableColumnCache — LRU cache (max 25 entries) mapping SQLite table name → ordered column names, populated on miss via PRAGMA_TABLE_INFO. Improves performance on repeated access.
    • ChangesetFilterContext — consolidates all filter and mode state (m_propertyFilter, m_strictMode, m_tableFilters, m_opcodeFilters, m_ecclassNameFilters) and exposes IsTableAllowed, IsOpcodeAllowed, IsECClassNameAllowed, CheckColumnCount, and Reset.
    • InternalChangeIterator — owns the ChangeStream / Changes / Change triple and drives low-level row iteration (Open, Advance, Reset, IsOpen, IsStepped).
  • Per-row output split into m_oldFields and m_newFields (previously a single m_fields map keyed by stage).
  • m_columnValues hoisted to a member (std::unordered_map<Utf8String, DbValue>) to avoid repeated re-allocation per step.
  • GetColumnValues(Stage) now writes into m_columnValues (member) rather than returning a map by reference.
  • ProcessStageValues now takes std::vector<Utf8String>* (nullable) for changedPropNames so callers can opt out of name collection.
  • CloseInfallible moved from public to private; called by destructor.
  • Filtering APIs on PreparedChangesetReader changed from inline void setters to proper BentleyStatus-returning methods with IsOpen() guard checks.
  • WriteGroupToFile now takes ECDbCR ecdb explicitly.
  • GetColumnCountForCurrentChangedTable now uses TableColumnCache for efficiency and is no longer const.
  • IsOpen() now checks m_ecdb != nullptr && m_iterator.IsOpen().

ChangesetValueFactory (internal)

  • All changedProps parameters across Create, BuildPropertyFields, CreatePrimitive, CreateSystem, CreatePoint2d, CreatePoint3d, CreateNav, CreateArray, CreateStruct, and CreateValueForProperty changed from std::vector<Utf8String>& (out-reference) to std::vector<Utf8String>* (nullable pointer).
  • New helper static void FillChangedPropIfApplicable(std::vector<Utf8String>* changedProps, Utf8String const& propName) — appends only when pointer is non-null; replaces all direct emplace_back calls.
  • CreateStruct hoists the memberTemp / memberChangedProps vectors outside the loop and uses .clear() to avoid per-iteration allocation.

Build & Headers

  • ChangesetReaderImpl.cpp and ChangesetReaderImpl.h deleted entirely.
  • ECDb.mke: removed ChangesetReaderImpl.cpp compilation rule and ChangesetReaderImpl.h from ECDbAllHeaders.
  • ECDbPch.h: removed #include "ChangesetReaderImpl.h".
  • <list> added to PreparedChangesetReader.h (required by std::list used in TableColumnCache).

Tests

  • Existing ChangesetReaderTests.cpp updated for the new GetChangeFetchedPropertyNames() pointer API (no out-parameter).
  • New cache-focused tests added for TableColumnCache behaviour.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR is a WIP that updates the native ChangesetReader plumbing in ECDb and changes the Node addon’s TypeScript-facing API to support batched/multi-row retrieval (returning metadata plus old/new stage values per row).

Changes:

  • Node addon: replaces per-row step()/getValue()/getChangeMetadata() usage with a single step(numOfRows, rowOptions) call that returns an array of { metadata, oldValues, newValues }.
  • ECDb: refactors ChangesetReader to own PreparedChangesetReader directly, introduces an LRU TableColumnCache, and changes GetChangeFetchedPropertyNames() to return a pointer (or nullptr) instead of filling an out-parameter.
  • Tests/build: updates existing tests for the new property-name API and adds cache-focused tests; removes the obsolete ChangesetReaderImpl files from build inputs.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 16 comments.

Show a summary per file
File Description
iModelJsNodeAddon/IModelJsNative.cpp Updates the JS projection of ChangesetReader to return batched rows with metadata + old/new values.
iModelJsNodeAddon/api_package/ts/src/NativeLibrary.ts Updates TypeScript declarations to the new batched ChangesetReader.step() signature and new row types.
iModelCore/ECDb/Tests/NonPublished/ChangesetReaderTests.cpp Adapts tests to new GetChangeFetchedPropertyNames() signature; adds new cache behavior tests.
iModelCore/ECDb/PublicAPI/ECDb/ChangesetReader.h Updates ChangesetReader public API to use PreparedChangesetReader and pointer-returning property-name list.
iModelCore/ECDb/ECDb/PreparedChangesetReader.h Adds TableColumnCache + filter context/iterator helpers; updates filtering APIs and property-name retrieval.
iModelCore/ECDb/ECDb/PreparedChangesetReader.cpp Implements TableColumnCache, filter context, iterator refactor, and updated row processing.
iModelCore/ECDb/ECDb/ECDbPch.h Removes include of deleted ChangesetReaderImpl.h.
iModelCore/ECDb/ECDb/ECDb.mke Removes deleted ChangesetReaderImpl.* from build inputs.
iModelCore/ECDb/ECDb/ChangesetValueFactory.h Changes changed-prop collection to optional pointer; adds helper to conditionally append.
iModelCore/ECDb/ECDb/ChangesetValueFactory.cpp Implements optional changed-prop collection and helper; minor comment tweaks needed.
iModelCore/ECDb/ECDb/ChangesetReaderImpl.h Deleted (obsolete wrapper).
iModelCore/ECDb/ECDb/ChangesetReaderImpl.cpp Deleted (obsolete wrapper).
iModelCore/ECDb/ECDb/ChangesetReader.cpp Refactors ChangesetReader to own PreparedChangesetReader directly; adds open checks/logging (but currently has critical status-handling bugs).

Comment thread iModelCore/ECDb/ECDb/ChangesetReader.cpp
Comment thread iModelCore/ECDb/ECDb/ChangesetReader.cpp
Comment thread iModelCore/ECDb/ECDb/ChangesetReader.cpp
Comment thread iModelCore/ECDb/ECDb/ChangesetReader.cpp
Comment thread iModelCore/ECDb/ECDb/ChangesetReader.cpp
Comment thread iModelCore/ECDb/ECDb/ChangesetValueFactory.cpp Outdated
Comment thread iModelCore/ECDb/ECDb/ChangesetValueFactory.cpp Outdated
Comment thread iModelCore/ECDb/ECDb/ChangesetValueFactory.cpp Outdated
Comment thread iModelCore/ECDb/ECDb/ChangesetValueFactory.cpp Outdated
Comment thread iModelJsNodeAddon/api_package/ts/src/NativeLibrary.ts
soham-bentley and others added 5 commits June 24, 2026 15:46
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ham/adding_multi-row_return_for_typescript_caching
@soham-bentley soham-bentley changed the title WIP: Adding support for multi row batched return for ChangesetReader typescript api Adding support for multi row batched return for ChangesetReader typescript api Jun 24, 2026
@soham-bentley soham-bentley marked this pull request as ready for review June 24, 2026 12:23
Comment thread iModelCore/ECDb/PublicAPI/ECDb/ChangesetReader.h Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comment thread iModelCore/ECDb/Tests/NonPublished/ChangesetReaderTests.cpp Outdated
Comment thread iModelCore/ECDb/PublicAPI/ECDb/ChangesetReader.h
@soham-bentley soham-bentley enabled auto-merge (squash) June 26, 2026 09:35
@khanaffan

Copy link
Copy Markdown
Contributor

This PR and its peer native PR collectively made changes that improve performance. But there is no number show what and how much this code contributed to improvements. It be nice we have that idea and also add that to NextVersion.md to advertise the improvements made to the api.

@soham-bentley

Copy link
Copy Markdown
Contributor Author

This PR and its peer native PR collectively made changes that improve performance. But there is no number show what and how much this code contributed to improvements. It be nice we have that idea and also add that to NextVersion.md to advertise the improvements made to the api.

Added to NextVersion.md the results after running perf test before and after

@soham-bentley

Copy link
Copy Markdown
Contributor Author

/azp run imodel-native

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…ham/adding_multi-row_return_for_typescript_caching
@aruniverse aruniverse added this to the iTwin.js 5.12 milestone Jun 29, 2026
…ham/adding_multi-row_return_for_typescript_caching
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChangesetReader performance improvements

5 participants