From 0229bd5b8e438c7f6ff85b8b203dfffc6ffa2426 Mon Sep 17 00:00:00 2001 From: fisehara Date: Wed, 28 Jun 2023 17:33:47 +0200 Subject: [PATCH] Optimize Duplications of `ComputedField` queries For each modelName the a `ComputedField` is only compiled into abstract-sql-query once. Afterwards it's used as ReferencedField Change-type: minor Signed-off-by: fisehara --- src/odata-to-abstract-sql.ts | 41 ++++++++++++---- test/chai-sql.js | 24 +++++++++ test/filterby.js | 94 ++++++++++++++++++++++++++++++++++++ test/model.sbvr | 8 ++- test/select.js | 78 ++++++++++++++++++++++++++++++ 5 files changed, 235 insertions(+), 10 deletions(-) diff --git a/src/odata-to-abstract-sql.ts b/src/odata-to-abstract-sql.ts index e1dd0dd..f3982e3 100644 --- a/src/odata-to-abstract-sql.ts +++ b/src/odata-to-abstract-sql.ts @@ -117,6 +117,8 @@ type Overwrite = Pick> & U; type RequiredField = Overwrite>>; type AliasedResource = RequiredField; +type AlreadyComputedFieldsLookup = { [resourceAndFieldName: string]: boolean }; + export type ResourceFunction = ( this: OData2AbstractSQL, property: { @@ -298,10 +300,17 @@ export const isBindReference = (maybeBind: { ); }; -const isDynamicResource = (resource: Resource): boolean => { +const isDynamicResource = ( + resource: Resource, + alreadyComputedFieldsLookup: AlreadyComputedFieldsLookup, +): boolean => { return ( resource.definition != null || - resource.fields.some((f) => f.computed != null) + resource.fields.some( + (f) => + f.computed != null && + !alreadyComputedFieldsLookup[resource.name + '$' + f.fieldName], + ) ); }; @@ -327,6 +336,7 @@ export class OData2AbstractSQL { public defaultResource: Resource | undefined; public bindVarsLength: number = 0; private checkAlias: (alias: string) => string; + private alreadyComputedFields: AlreadyComputedFieldsLookup = {}; constructor( private clientModel: RequiredAbstractSqlModelSubset, @@ -543,7 +553,9 @@ export class OData2AbstractSQL { // For updates/deletes that we use a `WHERE id IN (SELECT...)` subquery to apply options and in the case of a definition // we make sure to always apply it. This means that the definition will still be applied for these queries if ( - (hasQueryOpts || isDynamicResource(resource) || pathKeyWhere != null) && + (hasQueryOpts || + isDynamicResource(resource, this.alreadyComputedFields) || + pathKeyWhere != null) && (method === 'POST' || method === 'PUT-INSERT') ) { // For insert statements we need to use an INSERT INTO ... SELECT * FROM (binds) WHERE ... style query @@ -562,8 +574,9 @@ export class OData2AbstractSQL { 'SelectQuery', [ 'Select', - (resource.modifyFields ?? resource.fields).map( - (field): AliasNode => { + (resource.modifyFields ?? resource.fields) + .filter((field) => field.computed == null) + .map((field): AliasNode => { const alias = field.fieldName; const bindVar = bindVars?.find((v) => v[0] === alias); const value = bindVar?.[1] ?? ['Null']; @@ -573,8 +586,7 @@ export class OData2AbstractSQL { ); } return ['Alias', ['Cast', value, field.dataType], alias]; - }, - ), + }), ], ], '$insert', @@ -687,7 +699,8 @@ export class OData2AbstractSQL { // we make sure to always apply it. This means that the definition will still be applied for these queries, for insert queries // this is handled when we set the 'Values' if ( - (hasQueryOpts || isDynamicResource(resource)) && + (hasQueryOpts || + isDynamicResource(resource, this.alreadyComputedFields)) && (method === 'PUT' || method === 'PATCH' || method === 'MERGE' || @@ -991,11 +1004,18 @@ export class OData2AbstractSQL { fieldName: string, computed?: AbstractSqlQuery, alias: string = fieldName, + // Setting this flag to true will ignore the lookup for alreadyComputedFields + // and will compile the computed Field statement into the abstract SQL statement regardless + forceCompilingComputedField: boolean = false, ): | ReferencedFieldNode | AliasNode | AliasNode { - if (computed) { + const key = resource.name + '$' + fieldName; + if ( + computed && + (!this.alreadyComputedFields[key] || forceCompilingComputedField) + ) { if ( resource.tableAlias != null && resource.tableAlias !== resource.name @@ -1006,6 +1026,7 @@ export class OData2AbstractSQL { resource.tableAlias, ); } + this.alreadyComputedFields[key] = true; return ['Alias', computed, alias]; } const referencedField = this.ReferencedField(resource, fieldName); @@ -1632,6 +1653,7 @@ export class OData2AbstractSQL { this.putReset(); this.extraBodyVars = {}; this.extraBindVars = [] as unknown as ODataBinds; + this.alreadyComputedFields = {}; } putReset() { @@ -1695,6 +1717,7 @@ export class OData2AbstractSQL { sqlNameToODataName(field.fieldName), field.computed, field.fieldName, + true, ), ), ]; diff --git a/test/chai-sql.js b/test/chai-sql.js index 8ff5c36..784c23f 100644 --- a/test/chai-sql.js +++ b/test/chai-sql.js @@ -105,6 +105,20 @@ const generateClientModel = function (input) { const sbvrModel = fs.readFileSync(require.resolve('./model.sbvr'), 'utf8'); export const clientModel = generateClientModel(sbvrModel); +clientModel.tables['copilot'].fields.push({ + fieldName: 'is blocked', + dataType: 'Boolean', + // The cast is needed because AbstractSqlQuery cannot express a constant value. + computed: ['Boolean', false], +}); + +clientModel.tables['copilot'].fields.push({ + fieldName: 'rank', + dataType: 'Text', + // The cast is needed because AbstractSqlQuery cannot express a constant value. + computed: ['Text', 'Junior'], +}); + const odataNameToSqlName = (odataName) => odataName.replace(/__/g, '-').replace(/_/g, ' '); @@ -360,3 +374,13 @@ export let teamFields = [ ]; export let $count = [['Alias', ['Count', '*'], '$count']]; + +export let copilotFields = [ + ['Alias', ['ReferencedField', 'copilot', 'created at'], 'created_at'], + ['Alias', ['ReferencedField', 'copilot', 'modified at'], 'modified_at'], + ['ReferencedField', 'copilot', 'id'], + ['ReferencedField', 'copilot', 'person'], + ['ReferencedField', 'copilot', 'assists-pilot'], + ['Alias', ['ReferencedField', 'copilot', 'is blocked'], 'is_blocked'], + ['ReferencedField', 'copilot', 'rank'], +]; diff --git a/test/filterby.js b/test/filterby.js index 9966ac1..f1c47ec 100644 --- a/test/filterby.js +++ b/test/filterby.js @@ -1433,3 +1433,97 @@ run(function () { }), ); }); + +test(`/copilot?$select=id,rank&$filter=rank eq 'major'`, (result) => + it(`should get and filter copilot on computed field rank`, () => { + expect(result).to.be.a.query.to.deep.equal([ + 'SelectQuery', + [ + 'Select', + [ + ['ReferencedField', 'copilot', 'id'], + ['ReferencedField', 'copilot', 'rank'], + ], + ], + [ + 'From', + [ + 'Alias', + [ + 'SelectQuery', + [ + 'Select', + [ + ['Field', '*'], + ['Alias', ['Boolean', false], 'is blocked'], + ['Alias', ['Text', 'Junior'], 'rank'], + ], + ], + ['From', ['Table', 'copilot']], + ], + 'copilot', + ], + ], + [ + 'Where', + [ + 'IsNotDistinctFrom', + ['ReferencedField', 'copilot', 'rank'], + ['Bind', 0], + ], + ], + ]); + })); + +test( + `/copilot?$select=id,rank&$filter=rank eq 'major'`, + 'PATCH', + { assists__pilot: 1 }, + (result) => + it(`should PATCH copilot based on filtered computed field rank`, () => { + expect(result).to.be.a.query.to.deep.equal([ + 'UpdateQuery', + ['From', ['Table', 'copilot']], + [ + 'Where', + [ + 'In', + ['ReferencedField', 'copilot', 'id'], + [ + 'SelectQuery', + ['Select', [['ReferencedField', 'copilot', 'id']]], + [ + 'From', + [ + 'Alias', + [ + 'SelectQuery', + [ + 'Select', + [ + ['Field', '*'], + ['Alias', ['Boolean', false], 'is blocked'], + ['Alias', ['Text', 'Junior'], 'rank'], + ], + ], + ['From', ['Table', 'copilot']], + ], + 'copilot', + ], + ], + [ + 'Where', + [ + 'IsNotDistinctFrom', + ['ReferencedField', 'copilot', 'rank'], + ['Bind', 0], + ], + ], + ], + ], + ], + ['Fields', ['assists-pilot']], + ['Values', [['Bind', 'copilot', 'assists__pilot']]], + ]); + }), +); diff --git a/test/model.sbvr b/test/model.sbvr index cdce8bd..ed83575 100644 --- a/test/model.sbvr +++ b/test/model.sbvr @@ -57,4 +57,10 @@ Fact type: pilot has hire date Fact type: pilot1 was trained by pilot2 Synonymous Form: pilot2 trained pilot1 - Necessity: each pilot was trained by exactly one pilot \ No newline at end of file + Necessity: each pilot was trained by exactly one pilot + +Term: copilot + Concept Type: pilot + +Fact Type: copilot assists pilot + Necessity: each copilot assists at most one pilot diff --git a/test/select.js b/test/select.js index dfffe7b..08a42df 100644 --- a/test/select.js +++ b/test/select.js @@ -152,3 +152,81 @@ test('/pilot?$select=can_fly__plane/plane/id', (result) => ['ReferencedField', 'pilot.pilot-can fly-plane.plane', 'id'], ], ]))); + +test('/copilot?$select=*', (result) => + it('should select * from copilot', () => + expect(result).to.be.a.query.to.deep.equal([ + 'SelectQuery', + [ + 'Select', + [ + ['Alias', ['ReferencedField', 'copilot', 'created at'], 'created_at'], + [ + 'Alias', + ['ReferencedField', 'copilot', 'modified at'], + 'modified_at', + ], + ['ReferencedField', 'copilot', 'id'], + ['ReferencedField', 'copilot', 'pilot'], + [ + 'Alias', + ['ReferencedField', 'copilot', 'assists-pilot'], + 'assists__pilot', + ], + ['Alias', ['ReferencedField', 'copilot', 'is blocked'], 'is_blocked'], + ['ReferencedField', 'copilot', 'rank'], + ], + ], + [ + 'From', + [ + 'Alias', + [ + 'SelectQuery', + [ + 'Select', + [ + ['Field', '*'], + ['Alias', ['Boolean', false], 'is blocked'], + ['Alias', ['Text', 'Junior'], 'rank'], + ], + ], + ['From', ['Table', 'copilot']], + ], + 'copilot', + ], + ], + ]))); + +test('/copilot?$select=id,is_blocked,rank', (result) => + it('should select * from copilot', () => + expect(result).to.be.a.query.to.deep.equal([ + 'SelectQuery', + [ + 'Select', + [ + ['ReferencedField', 'copilot', 'id'], + ['Alias', ['ReferencedField', 'copilot', 'is blocked'], 'is_blocked'], + ['ReferencedField', 'copilot', 'rank'], + ], + ], + [ + 'From', + [ + 'Alias', + [ + 'SelectQuery', + [ + 'Select', + [ + ['Field', '*'], + ['Alias', ['Boolean', false], 'is blocked'], + ['Alias', ['Text', 'Junior'], 'rank'], + ], + ], + ['From', ['Table', 'copilot']], + ], + 'copilot', + ], + ], + ])));