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

Storage: Add extend_json Tag for Enhanced JSON Field Handling in bulk_update Operations #6659

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

rabbull
Copy link
Contributor

@rabbull rabbull commented Dec 10, 2024

In #6587, it was determined that when performing bulk_update operations on JSON fields like extra, the existing JSON should be extended rather than overwritten. To address this, this PR introduces a new tag, extend_json, that controls how bulk_update handles JSON fields. By default, extend_json is set to False, ensuring backward compatibility. When explicitly enabled, bulk_update merges the provided JSON with the existing JSON field, following to the RFC 7386 - JSON Merge Patch standard. This feature supports both SQLite and PostgreSQL backend.

Note:

  1. This implementation currently assumes that only attributes and extra are JSON fields of interest, and few additional JSON fields will be introduced. As a result, automatic detection of JSON fields is not implemented.
  2. The bulk_update logic for the SQLite DOS backend currently resides in PsqlDosBackend. A dirty workaround has to be added in PsqlDosBackend to address the differences between SQLite and PostgreSQL. This is also stated in a comment block in the code.

@rabbull rabbull requested a review from GeigerJ2 December 10, 2024 13:59
@rabbull rabbull self-assigned this Dec 10, 2024
@rabbull rabbull added the pr/work-in-progress PR that is still work in progress but already needs discussion label Dec 10, 2024
@unkcpz
Copy link
Member

unkcpz commented Dec 20, 2024

If some test hang up, it might benefit from #6674.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 88.70968% with 7 lines in your changes missing coverage. Please review.

Project coverage is 76.67%. Comparing base (f43a510) to head (eb3f17e).

Files with missing lines Patch % Lines
src/aiida/storage/psql_dos/utils.py 37.50% 5 Missing ⚠️
src/aiida/storage/psql_dos/backend.py 96.78% 1 Missing ⚠️
src/aiida/storage/sqlite_temp/backend.py 95.24% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6659      +/-   ##
==========================================
- Coverage   78.09%   76.67%   -1.41%     
==========================================
  Files         564      564              
  Lines       42544    42591      +47     
==========================================
- Hits        33219    32653     -566     
- Misses       9325     9938     +613     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rabbull rabbull removed the pr/work-in-progress PR that is still work in progress but already needs discussion label Jan 16, 2025
@unkcpz
Copy link
Member

unkcpz commented Jan 21, 2025

It seems the pytest hangs somewhere again. If we know which one is hang and can reproduce locally, I think I know how to debug it if it is the RMQ problem.

@rabbull
Copy link
Contributor Author

rabbull commented Jan 30, 2025

It seems the pytest hangs somewhere again. If we know which one is hang and can reproduce locally, I think I know how to debug it if it is the RMQ problem.

The failure seems related to my changes. I doubled the timeout limit and re-ran the CI pipelines to get a full output in which the exact failed tests are expected to show up.

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