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

Return metadata on executed Update #1675

Merged
merged 17 commits into from
Jan 23, 2025
Merged

Conversation

Qup42
Copy link
Member

@Qup42 Qup42 commented Dec 12, 2024

SPARQL Update requests sent via HTTP are now answered with a JSON object that contains useful metadata about the request, for example the number of changed triples and timing statistics.

Copy link

codecov bot commented Dec 15, 2024

Codecov Report

Attention: Patch coverage is 79.62963% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.89%. Comparing base (d7ec9be) to head (bdf45a7).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/engine/Server.cpp 78.12% 14 Missing ⚠️
src/index/DeltaTriples.cpp 75.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1675      +/-   ##
==========================================
- Coverage   89.90%   89.89%   -0.01%     
==========================================
  Files         393      393              
  Lines       37479    37565      +86     
  Branches     4221     4222       +1     
==========================================
+ Hits        33694    33770      +76     
- Misses       2482     2494      +12     
+ Partials     1303     1301       -2     

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

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

This looks very nice, I only have minor suggestions.
We'll have to try this out to check the format.

@Qup42 Qup42 marked this pull request as ready for review January 10, 2025 14:11
@Qup42 Qup42 requested a review from joka921 January 10, 2025 15:33
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

The code looks very clean now, only some miinor suggestions remain.
And @hannahbast could try it out to see if the response fits her expectations.

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Thank you very much! I have some minor suggestions + 1.5 questions.

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

This looks very great now, did @hannahbast sign off on the exact format in the meantime?
The only thing that remains is fixing the test failure (you changed a error message text, which is asserted).

@Qup42
Copy link
Member Author

Qup42 commented Jan 22, 2025

Yes we discussed it yesterday evening and Hannah approved. I didn't get to adjusting the tests between the changes in the meeting and now, but will do that in the next 3 hours.
This area still has the topics of returning the metadata in the format request with Accept, but I want to do a little research how other engines do this before inventing something new. The time breakdown doesn't currently add up (time is lost). Nothing jumped at me so fixing this is postponed, because this already provides significant value as-is and blocks some other PRs. (See the Issues in the GitHub Project for more detailed Info.)

@sparql-conformance
Copy link

@joka921 joka921 merged commit 4b7a260 into ad-freiburg:master Jan 23, 2025
22 of 24 checks passed
@Qup42 Qup42 deleted the updateMetadata branch January 24, 2025 12:16
Qup42 added a commit to Qup42/qlever that referenced this pull request Jan 24, 2025
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