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

feat(search): enable deep search with path expressions #590

Merged
merged 43 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
a194978
feat: deep search
patricebender Apr 16, 2024
6cd256e
comments
patricebender Apr 16, 2024
38652cb
add some end 2 end tests, remove caching
patricebender Apr 17, 2024
5a6f1ea
remove unused import
patricebender Apr 17, 2024
84963a0
skip calculated element tests for now
patricebender Apr 17, 2024
d2ae197
add test which adds new searchable fields dynamically
patricebender Apr 17, 2024
7d4ab5f
cosmetics
patricebender Apr 17, 2024
34b0ee3
Merge remote-tracking branch 'origin/main' into patrice/search
patricebender Apr 17, 2024
ab150ed
use inferred target for search resolving
patricebender Apr 22, 2024
82e73fa
Merge branch 'main' into patrice/search
patricebender Apr 22, 2024
11747f2
Merge branch 'main' into patrice/search
patricebender Apr 23, 2024
2521ff6
unskip tests, add todo
patricebender Apr 23, 2024
6982cea
Merge remote-tracking branch 'origin/main' into patrice/search
patricebender Apr 25, 2024
e6927b0
add search cache
patricebender Apr 25, 2024
f97e880
improve end2end tests
patricebender Apr 25, 2024
f667624
re-enable caching test
patricebender Apr 25, 2024
8cb5962
port everything to cqn4sql
patricebender Apr 27, 2024
7471bdb
minor adjustements
patricebender Apr 29, 2024
d2095e8
Merge branch 'main' into patrice/search
patricebender May 6, 2024
bce5054
we need cds@7.9.0
patricebender May 6, 2024
3a1c52f
Revert "we need cds@7.9.0"
patricebender May 7, 2024
e9eeed0
Merge remote-tracking branch 'origin/main' into patrice/search
patricebender May 7, 2024
bba5502
Merge branch 'main' into patrice/search
patricebender May 10, 2024
578cd0b
Merge branch 'main' into patrice/search
patricebender May 13, 2024
156c664
Merge branch 'main' into patrice/search
johannes-vogel May 13, 2024
9f8fd58
minor improvements
patricebender May 14, 2024
87b5f63
remove accidental change
patricebender May 14, 2024
5eb8ca0
return custom joins as is
patricebender May 14, 2024
e88113b
Merge branch 'main' into patrice/search
johannes-vogel Jun 7, 2024
4b3b7ef
infer custom join queries
patricebender Jun 7, 2024
fd559da
safeguard
patricebender Jun 7, 2024
53dbd1f
run all tests
patricebender Jun 7, 2024
98f3662
add another workaround
patricebender Jun 7, 2024
0ae8826
Merge branch 'main' into patrice/search
patricebender Jun 14, 2024
5809f04
Merge branch 'main' into patrice/search
johannes-vogel Jun 17, 2024
59578f6
expand subquery must be type of ql.Query
patricebender Jun 18, 2024
9f09795
Merge branch 'main' into patrice/search
patricebender Jun 18, 2024
d5d30e5
dont rely on cds.infer for expand subqueries
patricebender Jun 19, 2024
3cf4598
Merge branch 'main' into patrice/search
patricebender Jun 19, 2024
48ccb18
Merge branch 'main' into patrice/search
johannes-vogel Jul 5, 2024
6337006
Merge branch 'main' into patrice/search
johannes-vogel Jul 5, 2024
8b7ac43
apply suggestions
patricebender Jul 5, 2024
4dbcf83
move comment
patricebender Jul 5, 2024
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
2 changes: 2 additions & 0 deletions db-service/lib/SQLService.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ class SQLService extends DatabaseService {
* @type {Handler}
*/
async onSELECT({ query, data }) {
// REVISIT: for custom joins, infer is called twice, which is bad
// --> make cds.infer properly work with custom joins and remove this
if (!query.target) {
try { this.infer(query) } catch { /**/ }
}
Expand Down
134 changes: 80 additions & 54 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
'use strict'

const cds = require('@sap/cds')
const { computeColumnsToBeSearched } = require('./search')

const infer = require('./infer')
const { computeColumnsToBeSearched } = require('./search')

/**
* For operators of <eqOps>, this is replaced by comparing all leaf elements with null, combined with and.
Expand Down Expand Up @@ -44,8 +44,25 @@ const { pseudos } = require('./infer/pseudos')
* @returns {object} transformedQuery the transformed query
*/
function cqn4sql(originalQuery, model) {
const inferred = infer(originalQuery, model)
if (originalQuery.SELECT?.from.args && !originalQuery.joinTree) return inferred
let inferred = typeof originalQuery === 'string' ? cds.parse.cql(originalQuery) : cds.ql.clone(originalQuery)
patricebender marked this conversation as resolved.
Show resolved Hide resolved
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
if (!inferred.SELECT.elements) Object.setPrototypeOf(inferred, Object.getPrototypeOf(SELECT()))
const searchTerm = getSearchTerm(inferred.SELECT.search, inferred)
if (searchTerm) {
// Search target can be a navigation, in that case use _target to get the correct entity
const { where, having } = transformSearch(searchTerm)
if (where) inferred.SELECT.where = where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you need to append the search terms instead of replacing the existing where/having clauses?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done in line 243ff

else if (having) inferred.SELECT.having = having
}
}
inferred = infer(inferred, model)
// if the query has custom joins we don't want to transform it
// TODO: move all the way to the top of this function once cds.infer supports joins as well
// we need to infer the query even if no transformation will happen because cds.infer can't calculate the target
if (hasCustomJoins) return originalQuery

let transformedQuery = cds.ql.clone(inferred)
const kind = inferred.kind || Object.keys(inferred)[0]
Expand Down Expand Up @@ -111,7 +128,7 @@ function cqn4sql(originalQuery, model) {

// calculate the primary keys of the target entity, there is always exactly
// one query source for UPDATE / DELETE
const queryTarget = Object.values(originalQuery.sources)[0].definition
const queryTarget = Object.values(inferred.sources)[0].definition
const keys = Object.values(queryTarget.elements).filter(e => e.key === true)
const primaryKey = { list: [] }
keys.forEach(k => {
Expand Down Expand Up @@ -155,7 +172,7 @@ function cqn4sql(originalQuery, model) {
if (columns) {
transformedQuery.SELECT.columns = getTransformedColumns(columns)
} else {
transformedQuery.SELECT.columns = getColumnsForWildcard(originalQuery.SELECT?.excluding)
transformedQuery.SELECT.columns = getColumnsForWildcard(inferred.SELECT?.excluding)
}

// Like the WHERE clause, aliases from the SELECT list are not accessible for `group by`/`having` (in most DB's)
Expand All @@ -178,13 +195,6 @@ function cqn4sql(originalQuery, model) {
transformedQuery.SELECT.orderBy = transformedOrderBy
}
}

if (inferred.SELECT.search) {
// Search target can be a navigation, in that case use _target to get the correct entity
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 @@ -206,46 +216,29 @@ function cqn4sql(originalQuery, model) {
* 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 to be applied to the searchable columns within the query source.
* @param {object} searchTerm - 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|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.
* @returns {Object} - The function returns an object representing the WHERE or HAVING clause of the query.
*
* 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 transformSearch(search, from) {
const entity = getDefinition(from.$refLinks[0].definition.target) || from.$refLinks[0].definition
// 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 = {
func: 'search',
args: [
searchIn.length > 1 ? { list: searchIn } : { ...searchIn[0] },
xpr.length === 1 && 'val' in xpr[0] ? xpr[0] : { xpr },
],
}

// 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'

if (inferred.SELECT.groupBy && searchIn.some(c => c.func || c.xpr)) prop = 'having'
if (transformedQuery.SELECT[prop]) {
return { [prop]: [asXpr(transformedQuery.SELECT.where), 'and', contains] }
} else {
return { [prop]: [contains] }
}
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
const usesAggregation =
inferred.SELECT.groupBy &&
(searchTerm.args[0].func || searchTerm.args[0].xpr || searchTerm.args[0].list?.some(c => c.func || c.xpr))

if (usesAggregation) prop = 'having'
if (inferred.SELECT[prop]) {
return { [prop]: [asXpr(inferred.SELECT.where), 'and', searchTerm] }
} else {
return null
return { [prop]: [searchTerm] }
}
}

Expand Down Expand Up @@ -484,7 +477,7 @@ function cqn4sql(originalQuery, model) {
if (replaceWith === -1) transformedColumns.push(transformedColumn)
else transformedColumns.splice(replaceWith, 1, transformedColumn)

setElementOnColumns(transformedColumn, originalQuery.elements[col.as])
setElementOnColumns(transformedColumn, inferred.elements[col.as])
}

function getTransformedColumn(col) {
Expand Down Expand Up @@ -812,7 +805,8 @@ function cqn4sql(originalQuery, model) {

// we need to respect the aliases of the outer query, so the columnAlias might not be suitable
// as table alias for the correlated subquery
const uniqueSubqueryAlias = getNextAvailableTableAlias(columnAlias, originalQuery.outerQueries)
const uniqueSubqueryAlias = getNextAvailableTableAlias(columnAlias, inferred.outerQueries)

// `SELECT from Authors { books.genre as genreOfBooks { name } } becomes `SELECT from Books:genre as genreOfBooks`
const from = { ref: subqueryFromRef, as: uniqueSubqueryAlias }
const subqueryBase = Object.fromEntries(
Expand All @@ -830,7 +824,10 @@ function cqn4sql(originalQuery, model) {
}
const expanded = transformSubquery(subquery)
const correlated = _correlate({ ...expanded, as: columnAlias }, outerAlias)
Object.defineProperty(correlated, 'elements', { value: subquery.elements, writable: true })
Object.defineProperty(correlated, 'elements', {
value: expanded.elements,
writable: true,
})
return correlated

function _correlate(subq, outer) {
Expand Down Expand Up @@ -1051,7 +1048,7 @@ function cqn4sql(originalQuery, model) {
const last = q.SELECT.from.ref.at(-1)
const uniqueSubqueryAlias = inferred.joinTree.addNextAvailableTableAlias(
getLastStringSegment(last.id || last),
originalQuery.outerQueries,
inferred.outerQueries,
)
Object.defineProperty(q.SELECT.from, 'uniqueSubqueryAlias', { value: uniqueSubqueryAlias })
}
Expand Down Expand Up @@ -1659,7 +1656,7 @@ function cqn4sql(originalQuery, model) {
transformedFrom.as = from.as
} else {
// select from anonymous query, use artificial alias
transformedFrom.as = Object.keys(originalQuery.sources)[0]
transformedFrom.as = Object.keys(inferred.sources)[0]
}
return { transformedFrom, transformedWhere: existingWhere }
} else {
Expand Down Expand Up @@ -1702,7 +1699,7 @@ function cqn4sql(originalQuery, model) {
* --> This is an artificial query, which will later be correlated
* with the main query alias. see @function expandColumn()
*/
if (!(originalQuery.SELECT?.expand === true)) {
if (!(inferred.SELECT?.expand === true)) {
as = getNextAvailableTableAlias(as)
}
nextStepLink.alias = as
Expand Down Expand Up @@ -2162,6 +2159,35 @@ function cqn4sql(originalQuery, model) {
return getDefinition(assoc.target) || null
}

/**
* For a given search expression return a function "search" which holds the search expression
* as well as the searchable columns as arguments.
*
* @param {object} search - The search expression which shall be applied to the searchable columns on the query source.
* @param {object} query - The FROM clause of the CQN statement.
*
* @returns {(Object|null)} returns either:
* - a function with two arguments: The first one being the list of searchable columns, the second argument holds the search expression.
* - or null, if no searchable columns are found in neither in `@cds.search` nor in the target entity itself.
*/
function getSearchTerm(search, query) {
const entity = query.SELECT.from.SELECT ? query.SELECT.from : query.target
const searchIn = computeColumnsToBeSearched(inferred, entity)
if (searchIn.length > 0) {
const xpr = search
const searchFunc = {
func: 'search',
args: [
searchIn.length > 1 ? { list: searchIn } : { ...searchIn[0] },
xpr.length === 1 && 'val' in xpr[0] ? xpr[0] : { xpr },
],
}
return searchFunc
} else {
return null
}
}

/**
* Calculates the name of the source which can be used to address the given node.
*
Expand Down Expand Up @@ -2207,11 +2233,11 @@ function cqn4sql(originalQuery, model) {
* - but differs from the explicit alias, assigned by cqn4sql (i.e. <subquery>.from.uniqueSubqueryAlias)
*/
if (
originalQuery.SELECT?.from.uniqueSubqueryAlias &&
!originalQuery.SELECT?.from.as &&
inferred.SELECT?.from.uniqueSubqueryAlias &&
!inferred.SELECT?.from.as &&
firstStep === getLastStringSegment(transformedQuery.SELECT.from.ref[0])
) {
return originalQuery.SELECT?.from.uniqueSubqueryAlias
return inferred.SELECT?.from.uniqueSubqueryAlias
}
return node.ref[0]
}
Expand Down
2 changes: 1 addition & 1 deletion db-service/lib/infer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ for (const each in cdsTypes) cdsTypes[`cds.${each}`] = cdsTypes[each]
*/
function infer(originalQuery, model) {
if (!model) throw new Error('Please specify a model')
const inferred = typeof originalQuery === 'string' ? cds.parse.cql(originalQuery) : cds.ql.clone(originalQuery)
const inferred = originalQuery

// REVISIT: The more edge use cases we support, thes less optimized are we for the 90+% use cases
// e.g. there's a lot of overhead for infer( SELECT.from(Books) )
Expand Down
Loading
Loading