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(search): search on aggregated results in HAVING clause #524

Merged
merged 6 commits into from
Mar 14, 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
64 changes: 47 additions & 17 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,9 @@ function cqn4sql(originalQuery, model) {

if (inferred.SELECT.search) {
// Search target can be a navigation, in that case use _target to get the correct entity
const where = transformSearchToWhere(inferred.SELECT.search, transformedFrom)
if (where) {
transformedQuery.SELECT.where = where
}
const { where, having } = transformSearch(inferred.SELECT.search, transformedFrom) || {}
if (where) transformedQuery.SELECT.where = where
else if (having) transformedQuery.SELECT.having = having
}
return transformedQuery
}
Expand All @@ -204,20 +203,26 @@ function cqn4sql(originalQuery, model) {
}

/**
* Transforms a search expression to a WHERE clause for a SELECT operation.
* Transforms a search expression into a WHERE or HAVING clause for a SELECT operation, depending on the context of the query.
* The function decides whether to use a WHERE or HAVING clause based on the presence of aggregated columns in the search criteria.
*
* @param {object} search - The search expression which shall be applied to the searchable columns on the query source.
* @param {object} search - The search expression to be applied to the searchable columns within the query source.
* @param {object} from - The FROM clause of the CQN statement.
*
* @returns {(Object|Array|undefined)} - If the target of the query contains searchable elements, the function returns an array that represents the WHERE clause.
* If the SELECT query already contains a WHERE clause, this array includes the existing clause and appends an AND condition with the new 'contains' clause.
* If the SELECT query does not contain a WHERE clause, the returned array solely consists of the 'contains' clause.
* If the target entity of the query does not contain searchable elements, the function returns null.
* @returns {(Object|Array|null)} - The function returns an object representing the WHERE or HAVING clause of the query:
* - If the target of the query contains searchable elements, an array representing the WHERE or HAVING clause is returned.
* This includes appending to an existing clause with an AND condition or creating a new clause solely with the 'contains' clause.
* - If the SELECT query does not initially contain a WHERE or HAVING clause, the returned object solely consists of the 'contains' clause.
* - If the target entity of the query does not contain searchable elements, the function returns null.
*
* Note: The WHERE clause is used for filtering individual rows before any aggregation occurs.
* The HAVING clause is utilized for conditions on aggregated data, applied after grouping operations.
*/
function transformSearchToWhere(search, from) {
function transformSearch(search, from) {
const entity = getDefinition(from.$refLinks[0].definition.target) || from.$refLinks[0].definition
const searchIn = computeColumnsToBeSearched(inferred, entity, from.as)
// pass transformedQuery because we may need to search in the columns directly
// in case of aggregation
const searchIn = computeColumnsToBeSearched(transformedQuery, entity, from.as)
if (searchIn.length > 0) {
const xpr = search
const contains = {
Expand All @@ -228,10 +233,33 @@ function cqn4sql(originalQuery, model) {
],
}

if (transformedQuery.SELECT.where) {
return [asXpr(transformedQuery.SELECT.where), 'and', contains]
// 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
let prop = 'where'
// ANSI SQL aggregate functions
const aggregateFunctions = {
AVG: 1,
COUNT: 1,
MAX: 1,
MIN: 1,
SUM: 1,
EVERY: 1,
ANY: 1,
SOME: 1,
STDDEV_POP: 1,
STDDEV_SAMP: 1,
VAR_POP: 1,
VAR_SAMP: 1,
COLLECT: 1,
FUSION: 1,
INTERSECTION: 1,
}
if (inferred.SELECT.groupBy && searchIn.some(c => c.func in aggregateFunctions)) prop = 'having'
if (transformedQuery.SELECT[prop]) {
return { [prop]: [asXpr(transformedQuery.SELECT.where), 'and', contains] }
} else {
return [contains]
return { [prop]: [contains] }
}
} else {
return null
Expand Down Expand Up @@ -855,7 +883,8 @@ function cqn4sql(originalQuery, model) {
} else if (pseudos.elements[col.ref?.[0]]) {
res.push({ ...col })
} else if (col.ref) {
if (col.$refLinks.some(link => getDefinition(link.definition.target)?.['@cds.persistence.skip'] === true)) continue
if (col.$refLinks.some(link => getDefinition(link.definition.target)?.['@cds.persistence.skip'] === true))
continue
if (col.ref.length > 1 && col.ref[0] === '$self' && !col.$refLinks[0].definition.kind) {
const dollarSelfReplacement = calculateDollarSelfColumn(col)
res.push(...getTransformedOrderByGroupBy([dollarSelfReplacement], inOrderBy))
Expand Down Expand Up @@ -1749,7 +1778,8 @@ function cqn4sql(originalQuery, model) {
if (res === '$self')
// next is resolvable in entity
return prev
const definition = prev?.elements?.[res] || getDefinition(prev?.target)?.elements[res] || pseudos.elements[res]
const definition =
prev?.elements?.[res] || getDefinition(prev?.target)?.elements[res] || pseudos.elements[res]
const target = getParentEntity(definition)
thing.$refLinks[i] = { definition, target, alias: definition.name }
return prev?.elements?.[res] || getDefinition(prev?.target)?.elements[res] || pseudos.elements[res]
Expand Down
42 changes: 20 additions & 22 deletions db-service/lib/search.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const DEFAULT_SEARCHABLE_TYPE = 'cds.String'
/**
* This method gets all columns for an entity.
* It includes the generated foreign keys from managed associations, structured elements and complex and custom types.
* As well, it provides the annotations starting with '@' for each column.
* Moreover, it provides the annotations starting with '@' for each column.
*
* @param {object} entity - the csn entity
* @param {object} [options]
Expand Down Expand Up @@ -120,30 +120,28 @@ const computeColumnsToBeSearched = (cqn, entity = { __searchableColumns: [] }, a
// aggregations case
// in the new parser groupBy is moved to sub select.
if (cqn._aggregated || /* new parser */ cqn.SELECT.groupBy || cqn.SELECT?.from?.SELECT?.groupBy) {
cqn.SELECT.columns &&
cqn.SELECT.columns.forEach(column => {
if (column.func) {
// exclude $count by SELECT of number of Items in a Collection
if (
cqn.SELECT.columns.length === 1 &&
column.func === 'count' &&
(column.as === '_counted_' || column.as === '$count')
) {
return
}

toBeSearched.push(column)
cqn.SELECT.columns?.forEach(column => {
if (column.func) {
// exclude $count by SELECT of number of Items in a Collection
if (
cqn.SELECT.columns.length === 1 &&
column.func === 'count' &&
(column.as === '_counted_' || column.as === '$count')
) {
return
}

const columnRef = column.ref
if (columnRef) {
if (entity.elements[columnRef[columnRef.length - 1]]?._type !== DEFAULT_SEARCHABLE_TYPE) return
column = { ref: [...column.ref] }
if (alias) column.ref.unshift(alias)
toBeSearched.push(column)
}
})
toBeSearched.push({ func: column.func, args: column.args })
return
}

if (column.ref) {
if (entity.elements[column.ref.at(-1)]?._type !== DEFAULT_SEARCHABLE_TYPE) return
column = { ref: [...column.ref] }
if (alias) column.ref.unshift(alias)
toBeSearched.push(column)
}
})
} else {
toBeSearched = entity.own('__searchableColumns') || entity.set('__searchableColumns', _getSearchableColumns(entity))
if (cqn.SELECT.groupBy) toBeSearched = toBeSearched.filter(tbs => cqn.SELECT.groupBy.some(gb => gb.ref[0] === tbs))
Expand Down
13 changes: 13 additions & 0 deletions db-service/test/cqn4sql/search.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,17 @@ describe('Replace attribute search by search predicate', () => {
search((books.createdBy, books.modifiedBy, books.anotherText, books.title, books.descr, books.currency_code, books.dedication_text, books.dedication_sub_foo, books.dedication_dedication), ('x' OR 'y')) `,
)
})
it('Search with aggregated column and groupby must be put into having', () => {
// if we search on aggregated results, the search must be put into the having clause
const { Books } = cds.entities
let query = SELECT.from(Books)
.columns({ args: [{ ref: ['title'] }], as: 'firstInAlphabet', func: 'MIN' })
.groupBy('title')
.search('Cat')

expect(JSON.parse(JSON.stringify(cqn4sql(query, model)))).to.deep.equal(CQL`
SELECT from bookshop.Books as Books {
MIN(Books.title) as firstInAlphabet
} group by Books.title having search(MIN(Books.title), 'Cat')`)
})
})
1 change: 1 addition & 0 deletions test/scenarios/bookshop/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require('./search.test')
require('./read.test')
require('./insert.test')
require('./delete.test')
Expand Down
19 changes: 19 additions & 0 deletions test/scenarios/bookshop/search.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
const cds = require('../../cds.js')
const bookshop = require('path').resolve(__dirname, '../../bookshop')

describe('Bookshop - Search', () => {
const { expect } = cds.test(bookshop)

// search expression operating on aggregated results, must be put into the having clause
describe('with aggregate function', () => {
test('min', async () => {
const { Books } = cds.entities
let res = await SELECT.from(Books)
.columns({ args: [{ ref: ['title'] }], as: 'firstInAlphabet', func: 'MIN' })
.groupBy('title')
.search('Cat')
expect(res.length).to.be.eq(1)
})
})

})