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

PLU-818: [TILES-ATOMIC-INCREMENT-2] use .data() to automically update nested objects #818

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

pregnantboy
Copy link
Contributor

@pregnantboy pregnantboy commented Dec 16, 2024

TL;DR

Introduced a new patchTableRow function to atomically update specific columns in a table row while preserving other columns

What changed?

  • Currently, the update row action fetches the old data, merges the new changes and writes back.
  • This new implementation uses .data() to patch nested keys.

How to test?

  1. Create a table row with multiple columns
  2. Update only specific columns using the update-row action
  3. Verify that:
    • Only specified columns are updated
    • Non-existent columns are ignored
    • Other column values remain unchanged
    • Updates to non-existent rows fail gracefully
    • Number strings are automatically marshalled correctly

Why make this change?

The previous implementation required fetching the entire row before updating it, which could lead to race conditions and unnecessary data reads. The new atomic patch operation is more efficient and safer, as it updates only the specified columns in a single operation while preserving existing data.

@pregnantboy pregnantboy changed the title fix: use .data() to automically update nested objects PLU-818: [TILES-ATOMIC-INCREMENT-2] use .data() to automically update nested objects Dec 16, 2024
@pregnantboy pregnantboy marked this pull request as ready for review December 16, 2024 06:54
@pregnantboy pregnantboy requested a review from a team as a code owner December 16, 2024 06:54
Copy link
Contributor

@kevinkim-ogp kevinkim-ogp left a comment

Choose a reason for hiding this comment

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

lgtm, tested that update-row is working as expected 💪

@pregnantboy pregnantboy changed the base branch from feat/tiles/allow-copying-row-id to graphite-base/818 December 19, 2024 06:56
@pregnantboy pregnantboy force-pushed the fix/tiles/make-update-row-atomic branch from dcb0f39 to 70affe9 Compare December 19, 2024 06:57
@pregnantboy pregnantboy changed the base branch from graphite-base/818 to develop-v2 December 19, 2024 06:57
@pregnantboy pregnantboy force-pushed the fix/tiles/make-update-row-atomic branch from 70affe9 to 8a1843f Compare December 19, 2024 06:57
Copy link
Contributor Author

pregnantboy commented Dec 19, 2024

Merge activity

  • Dec 19, 2:02 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 19, 2:02 AM EST: A user merged this pull request with Graphite.

@pregnantboy pregnantboy merged commit 7e0773a into develop-v2 Dec 19, 2024
4 checks passed
@pregnantboy pregnantboy deleted the fix/tiles/make-update-row-atomic branch December 19, 2024 07:02
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.

2 participants