Adding support for caching inChangesetReader api#9435
Conversation
ChangesetReader apiChangesetReader api
ChangesetReader apiChangesetReader api
There was a problem hiding this comment.
Pull request overview
Adds native-side row batching/caching to the ChangesetReader iteration API, along with a new setBatchSize() tuning knob and tighter “configure-before-stepping” guards. Updates learning docs, change history, and examples/tests to reflect the new behavior and options.
Changes:
- Implement batched stepping in
ChangesetReaderand exposesetBatchSize(n)to tune native fetch/cache size. - Enforce that filters/strict mode (and now batch size) are configured before the first successful
step(). - Add/refresh documentation, change history notes, and examples; expand standalone backend tests for the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| example-code/snippets/src/backend/ChangesetReader-examples.test.ts | Adds snippet coverage for invert and setBatchSize, plus a small assertion using pcu.instanceCount. |
| docs/learning/backend/ChangesetReader.md | Documents invert, clarifies “configure-before-step” constraints, and adds batch size guidance/defaults. |
| docs/changehistory/NextVersion.md | Adds change history entry describing ChangesetReader.setBatchSize and defaults. |
| core/backend/src/test/standalone/ChangesetReader.test.ts | Adds new standalone tests for the “already stepped” guard, batched stepping exhaustion behavior, lazy getters, and a larger insert-many scenario. |
| core/backend/src/ChangesetReader.ts | Implements native batch caching, adds setBatchSize, converts inserted/deleted to lazy getters, and adds “already stepped” guards to configuration methods. |
…oham/adding-batching-in-changesetreader
…s: true set to 20 from 25
ChangesetReader apiChangesetReader api
…oham/adding-batching-in-changesetreader
…db field because it is not required
MichaelSwigerAtBentley
left a comment
There was a problem hiding this comment.
Reviewing on behalf of docs. Docs lgtm
| expect(() => reader.op).to.throw("no current row"); | ||
| }); | ||
|
|
||
| it("setBatchSize(1) produces same instances as default batch size", () => { |
There was a problem hiding this comment.
Are there any tests that check the default batch sizes? We would want that caught if it changes
There was a problem hiding this comment.
We don't have any getters for the default batch size.....the default values are internal concept to the changeset reader, so I am not sure how we can assert on default batch sizes. Do we really need to?
There was a problem hiding this comment.
Mike, were you referring to tests that mock native and check the behavior, to test if we actually use expected batch sizes, or are you suggesting just checking the default batch sizes?
The second sounds like the "assert a constant equals its own value" anti pattern, I'd vote against doing that here. It would tell us nothing except "someone changed the number".
There was a problem hiding this comment.
I meant that we test if we actually use expected default batch sizes. Making sure that the default that is set is actually in use, and is what is returned in scenarios where no value is passed. It might be more appropriate to live in native
…oham/adding-batching-in-changesetreader
…oham/adding-batching-in-changesetreader
imodel-native: iTwin/imodel-native#1474
Fixes https://github.com/iTwin/itwinjs-backlog/issues/2161
Summary
Adds native-side row batching/caching to the
ChangesetReaderiteration API in@itwin/core-backend. Introduces a newsetBatchSize(n)method to tune how many change rows are fetched and cached per native call, along with stricter "configure-before-stepping" guards for filters, strict mode, and the new batch size setting.Key Changes
New API —
ChangesetReader.setBatchSize(n: number)step()call — throwsIModelErrorif called after iteration begins.Default batch sizes (when
setBatchSizeis not called):propFilter: InstanceKeypropFilter: AllorBisCoreElement,abbreviateBlobs: falsepropFilter: AllorBisCoreElement(all other cases)Stricter configure-before-step guards
Filters (
setTableNameFilters,clearTableNameFilters, etc.), strict mode (enableStrictMode/disableStrictMode), andsetBatchSizemust all be configured before the first successfulstep()call. Previously, strict mode could be toggled mid-stream; this is no longer allowed.inserted/deletedconverted to gettersChangeInstance.insertedandChangeInstance.deletedchanged from plain optional properties to propergetaccessors in the public API surface (core-backend.api.md).Files Changed
core/backend/src/ChangesetReader.tssetBatchSize, stricter guardscore/backend/src/test/standalone/ChangesetReader.test.tscommon/api/core-backend.api.mdsetBatchSize, getter conversionsdocs/changehistory/NextVersion.mdChangesetReader.setBatchSizedocs/learning/backend/ChangesetReader.mdinvert, batch size section, filter/strict-mode guardsexample-code/snippets/src/backend/ChangesetReader-examples.test.tsInvertChangeset,SetBatchSize,instanceCountusagecommon/changes/@itwin/core-backend/...jsontype: none)Performance Trade-off
ChangesetReader — default batch size rationale
The rationale is explained in this comment in the attached work item