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

fix: query-patterns-should-not-be-separated DEV-2473 #2786

Merged
merged 10 commits into from
Aug 15, 2023

Conversation

seakayone
Copy link
Collaborator

@seakayone seakayone commented Aug 14, 2023

Pull Request Checklist

Task Description/Number

e351592 introduces a changed behaviour in how query patterns are transformed/optimized.

The create slow down reported in DEV-2473 is caused by this.
The gravsearch query which is used for retrieving the information of the resource to which a value should be created is containing a different optional block whilst the rest of the query still was the same.

The difference in runtime is enormous:
407 results in 0.083 seconds (fixed, old behaviour):

OPTIONAL {
  ?standoffNode <http://www.knora.org/ontology/knora-base#standoffTagHasInternalReference> ?targetStandoffTag .
  ?targetStandoffTag <http://www.knora.org/ontology/knora-base#standoffTagHasOriginalXMLID> ?targetOriginalXMLID .
}

and

88198 results in 5.43 seconds (faulty behaviour):

OPTIONAL {
  ?standoffNode <http://www.knora.org/ontology/knora-base#standoffTagHasInternalReference> ?targetStandoffTag .
}
OPTIONAL {
  ?targetStandoffTag <http://www.knora.org/ontology/knora-base#standoffTagHasOriginalXMLID> ?targetOriginalXMLID .
}

In the faulty example if a match is found for the first triple pattern, it will return results that include that match, even if there is no match for the second triple pattern, and vice versa.

Basic Requirements

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • fix: represents bug fixes
  • refactor: represents production code refactoring
  • feat: represents a new feature
  • docs: documentation changes (no production code change)
  • chore: maintenance tasks (no production code change)
  • test: all about tests: adding, refactoring tests (no production code change)
  • other... Please describe:

Does this PR introduce a breaking change?

  • Yes
  • No
  • Maybe (not 100% sure => check with FE)

Does this PR change client-test-data?

  • Yes (don't forget to update the JS-LIB team about the change)
  • No

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: +4.33% 🎉

Comparison is base (12402f3) 18.00% compared to head (ded9718) 22.33%.
Report is 33 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2786      +/-   ##
==========================================
+ Coverage   18.00%   22.33%   +4.33%     
==========================================
  Files         281      242      -39     
  Lines       28899    23394    -5505     
==========================================
+ Hits         5202     5225      +23     
+ Misses      23697    18169    -5528     

see 93 files with indirect coverage changes

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

@seakayone seakayone self-assigned this Aug 14, 2023
@seakayone seakayone changed the title optional-patterns-should-not-be-separated fix: query-patterns-should-not-be-separated DEV-2473 Aug 14, 2023
@linear
Copy link

linear bot commented Aug 14, 2023

DEV-2473 SAVE PROCESS SLOW

From Stefan Münnich (Webern):

Description

Saving a new entry or even a small change (typo) sometimes takes a long time (more than 10 seconds).

https://dasch.atlassian.net/jira/servicedesk/projects/DSQ/queues/custom/3/DSQ-221

The resource to reproduce the bug with the label "Guido Adler tritt in den Ruhestand": https://ark.dasch.swiss/ark:/72163/1/0806/9pCn9mtsTmSzenpSL4YIcQg.20230806T085533017083949Z

Property label: "andere Quellen"

Screenshot from Flavie's console log (Firefox, staging server):

Screenshot 2023-08-07 at 10.20.04.png

@seakayone seakayone marked this pull request as ready for review August 14, 2023 15:59
@seakayone seakayone force-pushed the fix/optional-patterns-should-not-be-separated branch from 11548d3 to 0863bf4 Compare August 15, 2023 06:16
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

Thanks for spotting and fixing!

At some point we had a convention to name tests that use ZIO-Test ...ZSpec.scala - not sure if we still do that? But if we do, you may want to rename the files.

@seakayone
Copy link
Collaborator Author

Thanks for spotting and fixing!

At some point we had a convention to name tests that use ZIO-Test ...ZSpec.scala - not sure if we still do that? But if we do, you may want to rename the files.

I am happily ignoring this naming convention. Eventually all of our spec should be migrated to zio-test anyway. Also in the webapi/test source set zio-test should be the only available test framework.

…avsearch/transformers/ConstructTransformerSpec.scala

Co-authored-by: Marcin Procyk <marcin.procyk@dasch.swiss>
@seakayone seakayone enabled auto-merge (squash) August 15, 2023 06:57
@seakayone seakayone merged commit 0aae817 into main Aug 15, 2023
13 checks passed
@seakayone seakayone deleted the fix/optional-patterns-should-not-be-separated branch August 15, 2023 08:13
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