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

refactor: api for adjusting the order #453

Merged
merged 4 commits into from Mar 19, 2024
Merged

Conversation

tea-artist
Copy link
Contributor

@tea-artist tea-artist commented Mar 15, 2024

  1. Removes the API that sets order directly
  2. Remove order from the return value
  3. Set the position according to the anchor point
  4. Automatic shuffling ensures that ordering can be performed repeatedly

if (!curView) {
throw new BadRequestException('View not found in the table');
}
await this.prismaService.$tx(async () => {

Choose a reason for hiding this comment

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

await this.prismaService.$tx(async () => {
  await Promise.all(views.map(async (view, i) => {
    await this.viewService.updateViewByOps(tableId, view.id, [
      ViewOpBuilder.editor.setViewProperty.build({
        key: 'order',
        newValue: i,
        oldValue: view.order,
      }),
    ]);
  }));
});

I think it should be like this. It would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review

A large number of concurrent operations in a single transaction will not yield much incremental benefit, and this is not a bottleneck

Choose a reason for hiding this comment

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

I think it is a bottleneck, It will await sequence updateViewByOps
and when using Promise.all, it will await the longest updateViewByOps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First off, from the user's perspective, there won't be a large number of views, so the speed difference is almost imperceptible. However, if there really is a large quantity, the inherent overhead of numerous promises could actually slow things down. In another feature we implemented, we adopted the method of copying to a temporary table for bulk updates, which, after testing, resulted in a more than tenfold improvement in speed and is currently the fastest method we've found.

Another point worth mentioning is that we also support sqlite, which does not support concurrent writing. Attempting this in end-to-end (e2e) tests with sqlite has led to some strange errors.

@tea-artist
Copy link
Contributor Author

I changed some of your code @caoxing9 , you might want to check out the new implementation

@tea-artist tea-artist marked this pull request as ready for review March 18, 2024 05:52
Copy link
Contributor

@caoxing9 caoxing9 left a comment

Choose a reason for hiding this comment

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

LGTM

@tea-artist tea-artist merged commit 7e222cb into develop Mar 19, 2024
5 checks passed
@tea-artist tea-artist deleted the refactor/row-order branch March 19, 2024 03:24
tea-artist added a commit that referenced this pull request Mar 20, 2024
* feat: support excel import (#431)

* feat: support `csv` import

* perf: optimise import memory expend

* fix: `openapi` upload type error

* test: compatible ci attachment download

* feat: add import file icon

* feat: support `excel` import

feat: limit excel file size

refactor: rewrite import font-end

* chore: add `sheetjs` license

* feat: update import url validate strategy

* test: add excel import e2e

* feat: add logger in import records

* fix: type and lint error

* chore: replace axios to node-fetch in nestjs

* test: update import-table e2e

* test: update import-table delay time

* chore: delete `sheetjs` license

* feat: add analyze loading

* chore: export upload-panel component

* feat: integrate github login (#432)

* feat: integrate gitHub login

* feat: dynamic social auth module

* feat: add github auth env validation

* chore: remove electron (#438)

* feat: credit check (#436)

* feat: credit check

* test: exceed credit limit

* refactor: more appropriate email template structure (#437)

* chore: make pnpm automatically install the specified version of Node.js (#433)

* chore: remove nvm step

* fix: the error of copying and pasting empty content (#443)

* fix: the error of copying and pasting empty content

* ci: fixed node version

* ci: pnpm newline handling

* feat: compatible `tsv` import (#446)

* fix: rename link results in illegal data (#448)

* fix: unexpected horizontal lines on login page (#447)

* test: import test file save in tmp dir (#449)

* docs: add deploy on Zeabur in Readme (#451)

* docs: add deploy on Zeabur in Readme

* Update README.md

* Update README.md

* build:  image support `multi-platforms` & app image support `csp-open` (#445)

* build: app image support `csp-open`

* feat: docker image multi-platforms support

* fix: allow default db address (#461)

* feat: forgot password and setup new password (#452)

* feat: forgot password and setup new password

* chore: remove useless code

* fix: use instance test context creator

* fix: type error

* fix: sdk build config for *.spec.tsx

* fix: execute repeat when import upload (#464)

* fix: notify jump url & upgrade deps (#465)

* fix: notify jump url & upgrade deps

* feat: update `PUBLIC_DATABASE_PROXY` env

* fix: ensure interface-provided 'orderBy' parameter overrides view's default sort (#463)

* fix: ensure interface-provided 'orderBy' parameter overrides view's default sort

* fix: existence of identical fields sorted, with the later overwriting the earlier

* chore: signin error alerts

* feat: support google oatuh2 (#466)

* refactor: api for adjusting the order (#453)

* fix: i18n

* docs: star distribution

* refactor: view order

* refactor: view+table+base updateOrder

* feat: ci add `coverallsapp/github-action` (#468)

* feat: ci add `coverallsapp/github-action`

* fix: vitest `coverage.reporter` config (#469)

* fix: vitest `coverage.reporter` config

* chore(example): update dockers examples (#474)

* docs: Update header titles for consistency (#455)

* docs: add one-click deploy on sealos. (#462)

Signed-off-by: zzjin <tczzjin@gmail.com>

* chore: Update constants.ts (#457)

minor fix

* refactor: row order (#473)

* refactor: row order

* fix: sqlite test

---------

Signed-off-by: zzjin <tczzjin@gmail.com>
Co-authored-by: Mike <caoxing9@gmail.com>
Co-authored-by: Boris <boris2code@outlook.com>
Co-authored-by: Anjorin Damilare <damilareanjorin1@gmail.com>
Co-authored-by: Yuhang <2312744987@qq.com>
Co-authored-by: Pengap <penganpingprivte@gmail.com>
Co-authored-by: emmanuel <154705254+codesmith-emmy@users.noreply.github.com>
Co-authored-by: zzjin <zzjin@users.noreply.github.com>
Co-authored-by: Ikko Eltociear Ashimine <eltociear@gmail.com>
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.

None yet

3 participants