search

ncbo / goo

Graph Oriented Objects (GOO) for Ruby. A RDF/SPARQL based ORM.
http://ncbo.github.io/goo/
Other
15 stars 6 forks source link

Refactor: Queries module code refactor #117

Closed syphax-bouazzouni closed 11 months ago

syphax-bouazzouni commented 2 years ago

What

I refactored the code of the function model_load (used to build the SPARQL query and map the results into models), from a function with 600 lines that don't follow the Single responsibility principle, to two main classes (with functions that have less than 100 lines). The first one (QueryBuilder) builds the select query and the second one (SolutionMapper) maps the query results into models.

This is just a code extraction and isolation, no functional change has been made to the code

Why

I think that this refactor will make the code more understandable for normal humans (and new newcomers) and more open for extension.

Tests

Pass all the tests of test_where.rb

syphax-bouazzouni commented 11 months ago

Hi,

It's great to see that this PR got merged(d5bb126). Thanks, @mdorf

Feel free to message me if tests did not pass or if you have found issues.

mdorf commented 11 months ago

Some tests ARE failing at the REGEX functionality. Looks like #116 introduced these lines:

            when :regex
              if  filter_operation.value.is_a?(String)
                filter_operations << "REGEX(STR(?#{filter_var.to_s}) , \"#{filter_operation.value.to_s}\")"
              end

But I can't find that code in #117.

The failure we are getting is:

  8) Error:
TestWhere#test_filter:
SPARQL::Client::MalformedQuery: 400 Parser error
This is a 4store SPARQL server v1.1.6

parser error: syntax error, unexpected REGEX, expecting ')' on line 1

    /Users/mdorf/dev/ncbo/sparql-client/lib/sparql/client.rb:398:in `block in response'
    /Users/mdorf/dev/ncbo/sparql-client/lib/sparql/client.rb:746:in `request'
    /Users/mdorf/dev/ncbo/sparql-client/lib/sparql/client.rb:395:in `response'
    /Users/mdorf/dev/ncbo/sparql-client/lib/sparql/client.rb:320:in `query'
    /Users/mdorf/dev/ncbo/sparql-client/lib/sparql/client.rb:249:in `block in call_query_method'
    /Users/mdorf/dev/ncbo/sparql-client/lib/sparql/client/query.rb:343:in `result'
    /Users/mdorf/dev/ncbo/sparql-client/lib/sparql/client/query.rb:336:in `each_solution'
    /Users/mdorf/dev/ncbo/goo/lib/goo/sparql/solutions_mapper.rb:55:in `map_each_solutions'
    /Users/mdorf/dev/ncbo/goo/lib/goo/sparql/loader.rb:97:in `model_load_sliced'
    /Users/mdorf/dev/ncbo/goo/lib/goo/sparql/loader.rb:28:in `model_load'
    /Users/mdorf/dev/ncbo/goo/lib/goo/sparql/queries.rb:55:in `model_load'
    /Users/mdorf/dev/ncbo/goo/lib/goo/base/where.rb:214:in `process_query_intl'
    /Users/mdorf/dev/ncbo/goo/lib/goo/base/where.rb:131:in `process_query'
    /Users/mdorf/dev/ncbo/goo/lib/goo/base/where.rb:282:in `all'
    /Users/mdorf/dev/ncbo/goo/test/test_where.rb:501:in `test_filter'
codecov-commenter commented 11 months ago

Codecov Report

Merging #117 (9f88e07) into master (fb203b0) will decrease coverage by 2.67%. Report is 1 commits behind head on master. The diff coverage is 83.47%.

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   85.27%   82.61%   -2.67%     
==========================================
  Files          20       24       +4     
  Lines        2038     2266     +228     
==========================================
+ Hits         1738     1872     +134     
- Misses        300      394      +94
Files Changed Coverage Δ
lib/goo/sparql/loader.rb 66.54% <66.54%> (ø)
lib/goo/sparql/mixins/query_pattern.rb 88.46% <88.46%> (ø)
lib/goo/sparql/query_builder.rb 94.53% <94.53%> (ø)
lib/goo/sparql/solutions_mapper.rb 95.69% <95.69%> (ø)
lib/goo.rb 76.88% <100.00%> (ø)
lib/goo/sparql/queries.rb 100.00% <100.00%> (+4.47%) :arrow_up:
lib/goo/sparql/sparql.rb 100.00% <100.00%> (ø)
syphax-bouazzouni commented 11 months ago

Hi, it is fixed with (https://github.com/ncbo/goo/pull/117/commits/9f88e0752354a4b1c44ddd6b0ca649b709d5c7eb) I think the issue was that when mergin the REGEX code got remvoed.

So now all the tests pass for 4store. But not for Alegrograph can you check If you understand those errors? Sorry, I don't have for now enough expertise on Alegrograph to debug that. Keep me in touch thanks.

alexskr commented 11 months ago

We have not made AllegroGraph tests to pass yet so please ignore that for the time being

alexskr commented 11 months ago

@syphax-bouazzouni thanks for the quick fix

alexskr commented 11 months ago

(https://github.com/ncbo/goo/commit/9f88e0752354a4b1c44ddd6b0ca649b709d5c7eb) fixes fourstore tests but introduces new type of an error in AllegroGraph

before: 102 tests, 1396 assertions, 8 failures, 0 errors, 16 skips After: 102 tests, 1333 assertions, 8 failures, 12 errors, 16 skips

Example of the new type of error:

  9) Error:
TestComplex::TestModelComplex#test_empty_pages:
SPARQL::Client::MalformedQuery: MALFORMED QUERY: Line 1, <franz:yes> does not appear to be a valid prefix expansion. Original error: Unknown query option "imposeImplicitBasicOrdering". Valid options are: activeConditionTags, aliasPrefix, alias_*, allowStreamingResults, augmentQueriesWithConstraints, authorizationBasic, bgpJoinVersusExtendFactor, cancelQueryOnWarnings, chunkProcessingAllowed, chunkProcessingMemory, chunkProcessingSize, clauseReorderer, computeStatistics, defaultAttributes, defaultDatasetBehavior, diskChunkRowCount, enableDuplicateReduction, engine, fullScanWarningSize, gatherAdditionalMemoryInformation, honeycombApiKey, honeycombDataset, inferenceMode, inlineSingleBGPOptionals, iterateDeleteCost, logBacktraceOnQueryFailure, logLineLength, logQuery, maximumSolutionsSize, maximumValuesCountForService, memoryExhaustionWarningPercentage, memoryLimit, mergeJoinAllowed, numberOfRowsScannablePerProbe, patternEstimator, presentationTimeZone, profileOutlineDepth, profileOutputFormat, profileQuery, queryEngine, queryTimeout, reorderDuringExecution, serviceRequestSize, serviceTimeout, slowQueryLogThreshold, solrQueryLimit, temporaryFileDiskSpace, temporaryFilesystemSpaceLimit, trustEncodedDatatypesForRangeQueries, trustPredicateTypeMappingsForRangeQueries, typevar_*, useNegationAsFailureTransform, usePredicateConstrainedUpiTypeInformation, usePredicateConstrainedXsdTypeInformation, usePropertyPathCache, userAttributes, useSelectionForOrderingSize, verbose, warmupStringTable.
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:398:in `block in response'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:746:in `request'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:395:in `response'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:320:in `query'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:249:in `block in call_query_method'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client/query.rb:343:in `result'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client/query.rb:336:in `each_solution'
    /home/runner/work/goo/goo/lib/goo/sparql/solutions_mapper.rb:51:in `map_each_solutions'
    /home/runner/work/goo/goo/lib/goo/sparql/loader.rb:97:in `model_load_sliced'
    /home/runner/work/goo/goo/lib/goo/sparql/loader.rb:28:in `model_load'
    /home/runner/work/goo/goo/lib/goo/sparql/queries.rb:55:in `model_load'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:192:in `process_query_intl'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:131:in `process_query'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:282:in `all'
    /home/runner/work/goo/goo/test/test_model_complex.rb:691:in `test_empty_pages'

 10) Error:
TestComplex::TestModelComplex#test_readonly_pages_with_include:
SPARQL::Client::MalformedQuery: MALFORMED QUERY: Line 1, <franz:yes> does not appear to be a valid prefix expansion. Original error: Unknown query option "imposeImplicitBasicOrdering". Valid options are: activeConditionTags, aliasPrefix, alias_*, allowStreamingResults, augmentQueriesWithConstraints, authorizationBasic, bgpJoinVersusExtendFactor, cancelQueryOnWarnings, chunkProcessingAllowed, chunkProcessingMemory, chunkProcessingSize, clauseReorderer, computeStatistics, defaultAttributes, defaultDatasetBehavior, diskChunkRowCount, enableDuplicateReduction, engine, fullScanWarningSize, gatherAdditionalMemoryInformation, honeycombApiKey, honeycombDataset, inferenceMode, inlineSingleBGPOptionals, iterateDeleteCost, logBacktraceOnQueryFailure, logLineLength, logQuery, maximumSolutionsSize, maximumValuesCountForService, memoryExhaustionWarningPercentage, memoryLimit, mergeJoinAllowed, numberOfRowsScannablePerProbe, patternEstimator, presentationTimeZone, profileOutlineDepth, profileOutputFormat, profileQuery, queryEngine, queryTimeout, reorderDuringExecution, serviceRequestSize, serviceTimeout, slowQueryLogThreshold, solrQueryLimit, temporaryFileDiskSpace, temporaryFilesystemSpaceLimit, trustEncodedDatatypesForRangeQueries, trustPredicateTypeMappingsForRangeQueries, typevar_*, useNegationAsFailureTransform, usePredicateConstrainedUpiTypeInformation, usePredicateConstrainedXsdTypeInformation, usePropertyPathCache, userAttributes, useSelectionForOrderingSize, verbose, warmupStringTable.
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:398:in `block in response'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:746:in `request'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:395:in `response'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:320:in `query'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:249:in `block in call_query_method'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client/query.rb:343:in `result'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client/query.rb:336:in `each_solution'
    /home/runner/work/goo/goo/lib/goo/sparql/solutions_mapper.rb:51:in `map_each_solutions'
    /home/runner/work/goo/goo/lib/goo/sparql/loader.rb:97:in `model_load_sliced'
    /home/runner/work/goo/goo/lib/goo/sparql/loader.rb:28:in `model_load'
    /home/runner/work/goo/goo/lib/goo/sparql/queries.rb:55:in `model_load'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:192:in `process_query_intl'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:131:in `process_query'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:282:in `all'
    /home/runner/work/goo/goo/test/test_model_complex.rb:736:in `test_readonly_pages_with_include'

...

 32) Error:
TestSChemaless::TestSchemaless#test_page_reuse_predicates:
SPARQL::Client::MalformedQuery: MALFORMED QUERY: Line 1, <franz:yes> does not appear to be a valid prefix expansion. Original error: Unknown query option "imposeImplicitBasicOrdering". Valid options are: activeConditionTags, aliasPrefix, alias_*, allowStreamingResults, augmentQueriesWithConstraints, authorizationBasic, bgpJoinVersusExtendFactor, cancelQueryOnWarnings, chunkProcessingAllowed, chunkProcessingMemory, chunkProcessingSize, clauseReorderer, computeStatistics, defaultAttributes, defaultDatasetBehavior, diskChunkRowCount, enableDuplicateReduction, engine, fullScanWarningSize, gatherAdditionalMemoryInformation, honeycombApiKey, honeycombDataset, inferenceMode, inlineSingleBGPOptionals, iterateDeleteCost, logBacktraceOnQueryFailure, logLineLength, logQuery, maximumSolutionsSize, maximumValuesCountForService, memoryExhaustionWarningPercentage, memoryLimit, mergeJoinAllowed, numberOfRowsScannablePerProbe, patternEstimator, presentationTimeZone, profileOutlineDepth, profileOutputFormat, profileQuery, queryEngine, queryTimeout, reorderDuringExecution, serviceRequestSize, serviceTimeout, slowQueryLogThreshold, solrQueryLimit, temporaryFileDiskSpace, temporaryFilesystemSpaceLimit, trustEncodedDatatypesForRangeQueries, trustPredicateTypeMappingsForRangeQueries, typevar_*, useNegationAsFailureTransform, usePredicateConstrainedUpiTypeInformation, usePredicateConstrainedXsdTypeInformation, usePropertyPathCache, userAttributes, useSelectionForOrderingSize, verbose, warmupStringTable.
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:398:in `block in response'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:746:in `request'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:395:in `response'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:320:in `query'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client.rb:249:in `block in call_query_method'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client/query.rb:343:in `result'
    /home/runner/work/goo/goo/vendor/bundle/ruby/2.7.0/bundler/gems/sparql-client-fb4a89b420f8/lib/sparql/client/query.rb:336:in `each_solution'
    /home/runner/work/goo/goo/lib/goo/sparql/solutions_mapper.rb:51:in `map_each_solutions'
    /home/runner/work/goo/goo/lib/goo/sparql/loader.rb:97:in `model_load_sliced'
    /home/runner/work/goo/goo/lib/goo/sparql/loader.rb:28:in `model_load'
    /home/runner/work/goo/goo/lib/goo/sparql/queries.rb:55:in `model_load'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:192:in `process_query_intl'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:131:in `process_query'
    /home/runner/work/goo/goo/lib/goo/base/where.rb:282:in `all'
    /home/runner/work/goo/goo/test/test_schemaless.rb:200:in `test_page_reuse_predicates'
syphax-bouazzouni commented 11 months ago

Hi @alexskr , @mdorf It seems the issue with Algregoraph, comes from this line https://github.com/ontoportal-lirmm/goo/blob/9f88e0752354a4b1c44ddd6b0ca649b709d5c7eb/lib/goo/sparql/solutions_mapper.rb#L38-L41

This version of Algregorapah does not know the used option imposeImplicitBasicOrdering

Removing those lines passes the tests but breaks others that are harder to debug.

alexskr commented 11 months ago

I re-run tests in my environment with rfe17161-7.3.1.fasl.patch in place and this particular issue got resolved.
currently published develop version of agraph (v7.4.0-devel-2023-06-15) doesn't include this patch. I expect AGraph v7.4.0 to contain this patch when it is released.

ncbo-deployer commented 11 months ago

Yes, this patch will be part of AllegroGraph v7.4

On Jul 30, 2023, at 4:34 PM, Alex Skrenchuk @.**@.>> wrote:

I re-run tests in my environment with rfe17161-7.3.1.fasl.patch in place and this particular issue got resolved. currently published develop version of agraph (v7.4.0-devel-2023-06-15) doesn't include this patch. I expect AGraph v7.4.0 to contain this patch when it is released.

— Reply to this email directly, view it on GitHubhttps://github.com/ncbo/goo/pull/117#issuecomment-1657294590, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAP2LDXX4NHHWIJBLKD6YNTXS3VQZANCNFSM5QNJTFOQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>

bioontology-admin mailing list @.**@.> https://mailman.stanford.edu/mailman/listinfo/bioontology-admin