From 77b79788931c9c45f156d54a4b2ec76ba9ba629a Mon Sep 17 00:00:00 2001 From: Patrice Bender Date: Mon, 8 Jul 2024 16:33:40 +0200 Subject: [PATCH] fix: optimize foreign key access for expand with aggregations (#734) this is another follow up for #708 -> if the expand column was a foreign key access, the column was not properly re-written which resulted in the reported error. we now make use of the common functionality which ensure proper column re-write. Along the way, support for structured columns was added as well. --- db-service/lib/cqn4sql.js | 38 ++++++++++++-------------- db-service/test/cqn4sql/expand.test.js | 28 +++++++++++++++++++ test/scenarios/bookshop/read.test.js | 5 ++++ 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/db-service/lib/cqn4sql.js b/db-service/lib/cqn4sql.js index e2bbcd76f..7ba757af2 100644 --- a/db-service/lib/cqn4sql.js +++ b/db-service/lib/cqn4sql.js @@ -45,7 +45,8 @@ const { pseudos } = require('./infer/pseudos') */ function cqn4sql(originalQuery, model) { let inferred = typeof originalQuery === 'string' ? cds.parse.cql(originalQuery) : cds.ql.clone(originalQuery) - const hasCustomJoins = originalQuery.SELECT?.from.args && (!originalQuery.joinTree || originalQuery.joinTree.isInitial) + const hasCustomJoins = + originalQuery.SELECT?.from.args && (!originalQuery.joinTree || originalQuery.joinTree.isInitial) if (!hasCustomJoins && inferred.SELECT?.search) { // we need an instance of query because the elements of the query are needed for the calculation of the search columns @@ -226,7 +227,7 @@ function cqn4sql(originalQuery, model) { */ function transformSearch(searchTerm) { let prop = 'where' - + // if the query is grouped and the queries columns contain an aggregate function, // we must put the search term into the `having` clause, as the search expression // is defined on the aggregated result, not on the individual rows @@ -878,7 +879,7 @@ function cqn4sql(originalQuery, model) { // to be attached to dummy query const elements = {} - const expandedColumns = column.expand.map(expand => { + const expandedColumns = column.expand.flatMap(expand => { const fullRef = [...baseRef, ...expand.ref] if (expand.expand) { @@ -898,13 +899,11 @@ function cqn4sql(originalQuery, model) { if (expand.as) { columnCopy.as = expand.as } - if (columnCopy.isJoinRelevant) { - const tableAlias = getQuerySourceName(columnCopy) - const name = calculateElementName(columnCopy) - columnCopy.ref = [tableAlias, name] - } - elements[expand.as || expand.ref.map(idOnly).join('_')] = columnCopy.$refLinks.at(-1).definition - return columnCopy + const res = getFlatColumnsFor(columnCopy, { tableAlias: getQuerySourceName(columnCopy) }) + res.forEach(c => { + elements[c.as || c.ref.at(-1)] = c.element + }) + return res }) const SELECT = { @@ -935,15 +934,6 @@ function cqn4sql(originalQuery, model) { if (isCalculatedOnRead(col.$refLinks?.[col.$refLinks.length - 1].definition)) { const calcElement = resolveCalculatedElement(col, true) res.push(calcElement) - } else if (col.isJoinRelevant) { - const tableAlias = getQuerySourceName(col) - const name = calculateElementName(col) - const transformedColumn = { - ref: [tableAlias, name], - } - if (col.sort) transformedColumn.sort = col.sort - if (col.nulls) transformedColumn.nulls = col.nulls - res.push(transformedColumn) } else if (pseudos.elements[col.ref?.[0]]) { res.push({ ...col }) } else if (col.ref) { @@ -1001,8 +991,14 @@ function cqn4sql(originalQuery, model) { */ if (inOrderBy && flatColumns.length > 1) throw new Error(`"${getFullName(leaf)}" can't be used in order by as it expands to multiple fields`) - if (col.nulls) flatColumns[0].nulls = col.nulls - if (col.sort) flatColumns[0].sort = col.sort + flatColumns.forEach(fc => { + if (col.nulls) + fc.nulls = col.nulls + if (col.sort) + fc.sort = col.sort + if (fc.as) + delete fc.as + }) res.push(...flatColumns) } else { let transformedColumn diff --git a/db-service/test/cqn4sql/expand.test.js b/db-service/test/cqn4sql/expand.test.js index f407b5a91..527904e82 100644 --- a/db-service/test/cqn4sql/expand.test.js +++ b/db-service/test/cqn4sql/expand.test.js @@ -1050,6 +1050,34 @@ describe('Expands with aggregations are special', () => { const res = cqn4sql(q, model) expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx) }) + it('aggregation with structure', () => { + const q = CQL`SELECT from bookshop.Authors as Authors { + ID, + books { dedication } + } group by books.dedication` + + const qx = CQL`SELECT from bookshop.Authors as Authors left join bookshop.Books as books on books.author_ID = Authors.ID { + Authors.ID, + (SELECT from DUMMY { books.dedication_addressee_ID, books.dedication_text, books.dedication_sub_foo, books.dedication_dedication }) as books + } group by books.dedication_addressee_ID, books.dedication_text, books.dedication_sub_foo, books.dedication_dedication` + qx.SELECT.columns[1].SELECT.from = null + const res = cqn4sql(q, model) + expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx) + }) + it('optimized foreign key access', () => { + const q = CQL`SELECT from bookshop.Books { + ID, + Books.author { name, ID } + } group by author.name, author.ID` + + const qx = CQL`SELECT from bookshop.Books as Books left join bookshop.Authors as author on author.ID = Books.author_ID { + Books.ID, + (SELECT from DUMMY { author.name, Books.author_ID }) as author + } group by author.name, Books.author_ID` + qx.SELECT.columns[1].SELECT.from = null + const res = cqn4sql(q, model) + expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx) + }) it('expand path with filter must be an exact match in group by', () => { const q = CQL`SELECT from bookshop.Books { Books.ID, diff --git a/test/scenarios/bookshop/read.test.js b/test/scenarios/bookshop/read.test.js index c3b78c6d2..e88c44449 100644 --- a/test/scenarios/bookshop/read.test.js +++ b/test/scenarios/bookshop/read.test.js @@ -44,6 +44,11 @@ describe('Bookshop - Read', () => { expect(res.data.value.length).to.be.eq(4) // As there are two books which have the same author }) + test('same as above, with foreign key optimization', async () => { + const res = await GET('/admin/Books?$apply=filter(title%20ne%20%27bar%27)/groupby((author/name, author/ID),aggregate(price with sum as totalAmount))', admin) + expect(res.data.value.length).to.be.eq(4) // As there are two books which have the same author + }) + test('Path expression', async () => { const q = CQL`SELECT title, author.name as author FROM sap.capire.bookshop.Books where author.name LIKE '%a%'` const res = await cds.run(q)