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: optimize foreign key access for expand with aggregations #734

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 17 additions & 21 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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 = {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
28 changes: 28 additions & 0 deletions db-service/test/cqn4sql/expand.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions test/scenarios/bookshop/read.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading