Skip to content

doc: clarify filter option of sqlite.database.applyChangeset#63515

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
aduh95:filter-changeset
May 26, 2026
Merged

doc: clarify filter option of sqlite.database.applyChangeset#63515
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
aduh95:filter-changeset

Conversation

@aduh95
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 commented May 23, 2026 •
edited
Loading

The current version is quite confusing, if not plain wrong. I was heavily inspired by https://sqlite.org/session.html#sqlite3changeset_apply for that change.

For reference, our xFilter is implemented in

node/src/node_sqlite.cc

Lines 2238 to 2242 in 89c414a

static int xFilter(void* pCtx, const char* zTab) {
auto ctx = static_cast<ConflictCallbackContext*>(pCtx);
if (!ctx->filterCallback) return 1;
return ctx->filterCallback(zTab) ? 1 : 0;
}

@aduh95
doc: clarify filter option of sqlite.database.applyChangeset
89c414a 
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 23, 2026

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. sqlite Issues and PRs related to the SQLite subsystem. labels May 23, 2026
@louwers
Copy link
Copy Markdown
Contributor

louwers commented May 23, 2026

Maybe we should update to sqlite3_changeset_apply_v3. It has less confusing behavior it looks like.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2026
@aduh95
Copy link
Copy Markdown
Contributor Author

aduh95 commented May 23, 2026

Maybe we should update to sqlite3_changeset_apply_v3. It has less confusing behavior it looks like.

I have no opinion, but in any case we probably want to land the doc fix ASAP

@aduh95 aduh95 added the lts-watch-v22.x PRs that may need to be released in v22.x label May 23, 2026
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2026
@nodejs-github-bot nodejs-github-bot merged commit 85d4a5f into nodejs:main May 26, 2026
33 of 34 checks passed
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 26, 2026

Landed in 85d4a5f

@aduh95 aduh95 deleted the filter-changeset branch May 26, 2026 13:53
aduh95 added a commit that referenced this pull request May 27, 2026
@aduh95
doc: clarify filter option of sqlite.database.applyChangeset
0355c36 
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #63515
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrig cjihrig cjihrig approved these changes

@geeksilva97 geeksilva97 geeksilva97 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. lts-watch-v22.x PRs that may need to be released in v22.x sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aduh95 @nodejs-github-bot @louwers @cjihrig @geeksilva97