From 6825c0ca10f5b2b6f98a3a6f6915292ca6205229 Mon Sep 17 00:00:00 2001 From: Amir Sarabadani Date: Mon, 25 Jan 2021 09:10:11 +0100 Subject: [PATCH 1/2] Change the way negation works in "NotMatching" mode See the bug for the discussion Bug: T272140 --- src/sparql/QueryObjectBuilder.ts | 30 ++++++++++++----- tests/unit/sparql/QueryObjectBuilder.spec.ts | 6 ++++ tests/unit/sparql/buildQuery.spec.ts | 35 +++++++++++--------- 3 files changed, 47 insertions(+), 24 deletions(-) diff --git a/src/sparql/QueryObjectBuilder.ts b/src/sparql/QueryObjectBuilder.ts index 2eafc43b..b2fc6a1e 100644 --- a/src/sparql/QueryObjectBuilder.ts +++ b/src/sparql/QueryObjectBuilder.ts @@ -9,6 +9,7 @@ export default class QueryObjectBuilder { public constructor() { this.queryObject = { queryType: 'SELECT', + distinct: true, variables: [], where: [], type: 'query', @@ -214,18 +215,29 @@ export default class QueryObjectBuilder { if ( condition.propertyValueRelation === PropertyValueRelation.NotMatching ) { const filterCondition = { - type: 'filter', - expression: { - type: 'operation', - operator: '!=', - args: [ + type: 'minus', + patterns: [ { + type: 'bgp', + triples: [ { - termType: 'Variable', - value: 'instance', + subject: { + termType: 'Variable', + value: 'item', + }, + predicate: { type: 'path', + pathType: '/', + items: [ { + termType: 'NamedNode', + value: rdfNamespaces.p + condition.propertyId, + }, + { + termType: 'NamedNode', + value: rdfNamespaces.ps + condition.propertyId, + } ] }, + object: this.buildTripleObjectForExplicitValue( condition.datatype, condition.value ), }, - this.buildTripleObjectForExplicitValue( condition.datatype, condition.value ), ], - }, + } ], }; this.queryObject.where.push( filterCondition as Pattern ); diff --git a/tests/unit/sparql/QueryObjectBuilder.spec.ts b/tests/unit/sparql/QueryObjectBuilder.spec.ts index 40614054..f6afb569 100644 --- a/tests/unit/sparql/QueryObjectBuilder.spec.ts +++ b/tests/unit/sparql/QueryObjectBuilder.spec.ts @@ -8,6 +8,7 @@ describe( 'QueryObjectBuilder', () => { const builder = new QueryObjectBuilder(); const expected = { queryType: 'SELECT', + distinct: true, variables: [ { termType: 'Variable', @@ -68,6 +69,7 @@ describe( 'QueryObjectBuilder', () => { const builder = new QueryObjectBuilder(); const expected = { queryType: 'SELECT', + distinct: true, variables: [ { termType: 'Variable', @@ -130,6 +132,7 @@ describe( 'QueryObjectBuilder', () => { const builder = new QueryObjectBuilder(); const expected = { queryType: 'SELECT', + distinct: true, variables: [ { termType: 'Variable', @@ -221,6 +224,7 @@ describe( 'QueryObjectBuilder', () => { const builder = new QueryObjectBuilder(); const expected = { queryType: 'SELECT', + distinct: true, variables: [ { termType: 'Variable', @@ -290,6 +294,7 @@ describe( 'QueryObjectBuilder', () => { const builder = new QueryObjectBuilder(); const expected = { queryType: 'SELECT', + distinct: true, variables: [ { termType: 'Variable', @@ -374,6 +379,7 @@ describe( 'QueryObjectBuilder', () => { const builder = new QueryObjectBuilder(); const expected = { queryType: 'SELECT', + distinct: true, variables: [ { termType: 'Variable', diff --git a/tests/unit/sparql/buildQuery.spec.ts b/tests/unit/sparql/buildQuery.spec.ts index 3a6d74e9..3610b9ac 100644 --- a/tests/unit/sparql/buildQuery.spec.ts +++ b/tests/unit/sparql/buildQuery.spec.ts @@ -26,7 +26,7 @@ describe( 'buildQuery', () => { getSimpleCondition( propertyId, value ), ], omitLabels: true, - } ) ).toEqual( `SELECT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}) "${value}". }` ); + } ) ).toEqual( `SELECT DISTINCT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}) "${value}". }` ); } ); it( 'builds a query from a property and a string value with not matching selected', () => { @@ -36,9 +36,9 @@ describe( 'buildQuery', () => { condition.propertyValueRelation = PropertyValueRelation.NotMatching; const expectedQuery = - `SELECT ?item WHERE { + `SELECT DISTINCT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}) ?instance. - FILTER(?instance != "${value}") + MINUS { ?item (p:P666/ps:P666) "${value}". } }`; const receivedQuery = buildQuery( { @@ -53,16 +53,21 @@ describe( 'buildQuery', () => { const propertyId = 'P666'; const condition = getSimpleCondition( propertyId, '' ); condition.propertyValueRelation = PropertyValueRelation.Regardless; - - expect( buildQuery( { + const expectedQuery = + `SELECT DISTINCT ?item WHERE { + ?item (p:${propertyId}/ps:${propertyId}) _:anyValue${propertyId}. + }`; + const actualQuery = buildQuery( { conditions: [ condition ], omitLabels: true, - } ) ).toEqual( `SELECT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}) _:anyValue${propertyId}. }` ); + } ); + + expect( actualQuery.replace( /\s+/g, ' ' ) ).toEqual( expectedQuery.replace( /\s+/g, ' ' ) ); } ); it( 'builds a query from multiple conditions, one matching and one regardless', () => { const expectedQuery = - `SELECT ?item WHERE { + `SELECT DISTINCT ?item WHERE { ?item (p:P666/ps:P666) "blah". ?item (p:P66/ps:P66) _:anyValueP66. }`; @@ -81,10 +86,10 @@ describe( 'buildQuery', () => { it( 'builds a query from multiple conditions, one matching and one non-matching', () => { const expectedQuery = - `SELECT ?item WHERE { + `SELECT DISTINCT ?item WHERE { ?item (p:P666/ps:P666) "blah". ?item (p:P66/ps:P66) ?instance. - FILTER(?instance != "foo") + MINUS { ?item (p:P66/ps:P66) "foo". } }`; const secondCondition = getSimpleCondition( 'P66', 'foo' ); secondCondition.propertyValueRelation = PropertyValueRelation.NotMatching; @@ -101,9 +106,9 @@ describe( 'buildQuery', () => { it( 'builds a query from multiple conditions, one non-matching and one matching', () => { const expectedQuery = - `SELECT ?item WHERE { + `SELECT DISTINCT ?item WHERE { ?item (p:P666/ps:P666) ?instance. - FILTER(?instance != "blah") + MINUS { ?item (p:P666/ps:P666) "blah". } ?item (p:P66/ps:P66) "foo". }`; const firstCondition = getSimpleCondition( 'P666', 'blah' ); @@ -127,14 +132,14 @@ describe( 'buildQuery', () => { expect( buildQuery( { conditions: [ condition ], omitLabels: true, - } ) ).toEqual( `SELECT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}) wd:${value}. }` ); + } ) ).toEqual( `SELECT DISTINCT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}) wd:${value}. }` ); } ); it( 'builds a query from a property and a string value with omitLabels set to false (shows labels)', () => { const propertyId = 'P666'; const value = 'blah'; const expectedQuery = - `SELECT ?item ?itemLabel WHERE { + `SELECT DISTINCT ?item ?itemLabel WHERE { SERVICE wikibase:label { bd:serviceParam wikibase:language "[AUTO_LANGUAGE]". } ?item (p:P666/ps:P666) "blah". }`; const actualQuery = buildQuery( { @@ -149,7 +154,7 @@ describe( 'buildQuery', () => { const value = 'blah'; const subclassesId = process.env.VUE_APP_SUBCLASS_PROPERTY; const expectedQuery = - `SELECT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}/(wdt:${subclassesId}*)) "${value}". }`; + `SELECT DISTINCT ?item WHERE { ?item (p:${propertyId}/ps:${propertyId}/(wdt:${subclassesId}*)) "${value}". }`; const condition = getSimpleCondition( propertyId, value ); condition.subclasses = true; @@ -165,7 +170,7 @@ describe( 'buildQuery', () => { const propertyId = 'P666'; const value = 'blah'; const expectedQuery = - `SELECT ?item WHERE { + `SELECT DISTINCT ?item WHERE { MINUS { ?item (p:${propertyId}/ps:${propertyId}) "${value}". } ?item wikibase:sitelinks _:anyValue. }`; const condition = getSimpleCondition( propertyId, value ); From 89ab46340606bb6c936e46f18f2a5ce05ee25b4c Mon Sep 17 00:00:00 2001 From: Amir Sarabadani Date: Mon, 25 Jan 2021 10:17:43 +0100 Subject: [PATCH 2/2] Rework labelling to improve its performance Basically wrapping the actual query with the labelling service, see the discussion in T212933 and T166139 Bug: T272490 --- src/sparql/QueryObjectBuilder.ts | 107 ++++++++------ tests/unit/sparql/QueryObjectBuilder.spec.ts | 148 ++++++++++++------- tests/unit/sparql/buildQuery.spec.ts | 2 +- 3 files changed, 158 insertions(+), 99 deletions(-) diff --git a/src/sparql/QueryObjectBuilder.ts b/src/sparql/QueryObjectBuilder.ts index b2fc6a1e..6f9463ae 100644 --- a/src/sparql/QueryObjectBuilder.ts +++ b/src/sparql/QueryObjectBuilder.ts @@ -26,48 +26,6 @@ export default class QueryObjectBuilder { }, ]; - if ( !queryRepresentation.omitLabels ) { - this.queryObject.variables.push( - { - termType: 'Variable', - value: 'itemLabel', - } ); - - if ( !this.queryObject.where ) { - this.queryObject.where = []; - } - - this.queryObject.where.push( - { - type: 'service', - patterns: [ - { - type: 'bgp', - triples: [ { - subject: { - termType: 'NamedNode', - value: rdfNamespaces.bd + 'serviceParam', - }, - predicate: { - termType: 'NamedNode', - value: rdfNamespaces.wikibase + 'language', - }, - object: { - termType: 'Literal', - value: '[AUTO_LANGUAGE]', - }, - } ], - }, - ], - name: { - termType: 'NamedNode', - value: rdfNamespaces.wikibase + 'label', - }, - silent: false, - }, - ); - } - for ( let i = 0; i < queryRepresentation.conditions.length; i++ ) { this.buildFromQueryCondition( queryRepresentation.conditions[ i ] ); } @@ -77,7 +35,7 @@ export default class QueryObjectBuilder { let isOnlyNegateQuery = true; for ( let i = 0; i < this.queryObject.where?.length; i++ ) { const type = this.queryObject.where[ i ].type; - if ( type !== 'minus' && type !== 'service' ) { + if ( type !== 'minus' ) { isOnlyNegateQuery = false; break; } @@ -112,6 +70,10 @@ export default class QueryObjectBuilder { this.queryObject.limit = queryRepresentation.limit; } + if ( !queryRepresentation.omitLabels ) { + return this.wrapQueryWithLabel(); + } + return this.queryObject; } @@ -243,4 +205,63 @@ export default class QueryObjectBuilder { this.queryObject.where.push( filterCondition as Pattern ); } } + + private wrapQueryWithLabel(): SelectQuery { + const wrapperQuery: SelectQuery = { + queryType: 'SELECT', + distinct: true, + variables: [ + { + termType: 'Variable', + value: 'item', + }, + { + termType: 'Variable', + value: 'itemLabel', + }, + ], + where: [ + { + type: 'service', + patterns: [ + { + type: 'bgp', + triples: [ { + subject: { + termType: 'NamedNode', + value: rdfNamespaces.bd + 'serviceParam', + }, + predicate: { + termType: 'NamedNode', + value: rdfNamespaces.wikibase + 'language', + }, + object: { + termType: 'Literal', + value: '[AUTO_LANGUAGE]', + }, + } ], + }, + ], + name: { + termType: 'NamedNode', + value: rdfNamespaces.wikibase + 'label', + }, + silent: false, + }, + ], + type: 'query', + prefixes: rdfNamespaces, + }; + + if ( !wrapperQuery.where ) { + wrapperQuery.where = []; + } + this.queryObject.prefixes = {}; + wrapperQuery.where.push( { + type: 'group', + patterns: [ this.queryObject ], + } ); + + return wrapperQuery; + } } diff --git a/tests/unit/sparql/QueryObjectBuilder.spec.ts b/tests/unit/sparql/QueryObjectBuilder.spec.ts index f6afb569..1808f2ec 100644 --- a/tests/unit/sparql/QueryObjectBuilder.spec.ts +++ b/tests/unit/sparql/QueryObjectBuilder.spec.ts @@ -130,6 +130,46 @@ describe( 'QueryObjectBuilder', () => { it( 'with labels (omitLabels = false)', () => { const prefixes = allNamespaces; const builder = new QueryObjectBuilder(); + const internalExpectedQuery = { + queryType: 'SELECT', + distinct: true, + variables: [ + { + termType: 'Variable', + value: 'item', + }, + ], + where: [ + { + type: 'bgp', + triples: [ + { + subject: { + termType: 'Variable', + value: 'item', + }, + predicate: { type: 'path', + pathType: '/', + items: [ { + termType: 'NamedNode', + value: 'http://www.wikidata.org/prop/P281', + }, + { + termType: 'NamedNode', + value: 'http://www.wikidata.org/prop/statement/P281', + }, + ] }, + object: { + termType: 'Literal', + value: 'XXXX', + }, + }, + ], + }, + ], + type: 'query', + }; + const expected = { queryType: 'SELECT', distinct: true, @@ -172,29 +212,9 @@ describe( 'QueryObjectBuilder', () => { silent: false, }, { - type: 'bgp', - triples: [ - { - subject: { - termType: 'Variable', - value: 'item', - }, - predicate: { type: 'path', - pathType: '/', - items: [ { - termType: 'NamedNode', - value: 'http://www.wikidata.org/prop/P281', - }, - { - termType: 'NamedNode', - value: 'http://www.wikidata.org/prop/statement/P281', - }, - ] }, - object: { - termType: 'Literal', - value: 'XXXX', - }, - }, + type: 'group', + patterns: [ + internalExpectedQuery, ], }, ], @@ -377,7 +397,7 @@ describe( 'QueryObjectBuilder', () => { it( 'with negate but labels enabled', () => { const prefixes = allNamespaces; const builder = new QueryObjectBuilder(); - const expected = { + const internalExpectedQuery = { queryType: 'SELECT', distinct: true, variables: [ @@ -385,39 +405,8 @@ describe( 'QueryObjectBuilder', () => { termType: 'Variable', value: 'item', }, - { - termType: 'Variable', - value: 'itemLabel', - }, ], where: [ - { - type: 'service', - patterns: [ - { - type: 'bgp', - triples: [ { - subject: { - termType: 'NamedNode', - value: 'http://www.bigdata.com/rdf#serviceParam', - }, - predicate: { - termType: 'NamedNode', - value: 'http://wikiba.se/ontology#language', - }, - object: { - termType: 'Literal', - value: '[AUTO_LANGUAGE]', - }, - } ], - }, - ], - name: { - termType: 'NamedNode', - value: 'http://wikiba.se/ontology#label', - }, - silent: false, - }, { type: 'minus', patterns: [ @@ -470,6 +459,55 @@ describe( 'QueryObjectBuilder', () => { }, ], type: 'query', + prefixes: {}, + }; + const expected = { + queryType: 'SELECT', + distinct: true, + variables: [ + { + termType: 'Variable', + value: 'item', + }, + { + termType: 'Variable', + value: 'itemLabel', + }, + ], + where: [ + { + type: 'service', + patterns: [ + { + type: 'bgp', + triples: [ { + subject: { + termType: 'NamedNode', + value: 'http://www.bigdata.com/rdf#serviceParam', + }, + predicate: { + termType: 'NamedNode', + value: 'http://wikiba.se/ontology#language', + }, + object: { + termType: 'Literal', + value: '[AUTO_LANGUAGE]', + }, + } ], + }, + ], + name: { + termType: 'NamedNode', + value: 'http://wikiba.se/ontology#label', + }, + silent: false, + }, + { + type: 'group', + patterns: [ internalExpectedQuery ], + }, + ], + type: 'query', prefixes: prefixes, }; diff --git a/tests/unit/sparql/buildQuery.spec.ts b/tests/unit/sparql/buildQuery.spec.ts index 3610b9ac..5a322ce3 100644 --- a/tests/unit/sparql/buildQuery.spec.ts +++ b/tests/unit/sparql/buildQuery.spec.ts @@ -141,7 +141,7 @@ describe( 'buildQuery', () => { const expectedQuery = `SELECT DISTINCT ?item ?itemLabel WHERE { SERVICE wikibase:label { bd:serviceParam wikibase:language "[AUTO_LANGUAGE]". } - ?item (p:P666/ps:P666) "blah". }`; + { SELECT DISTINCT ?item WHERE { ?item (p:P666/ps:P666) "blah". } } }`; const actualQuery = buildQuery( { conditions: [ getSimpleCondition( propertyId, value ) ], omitLabels: false,