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.

Sign up for GitHub

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: cqn transformation is OData agnostic #990

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: cqn transformation is OData agnostic #990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8318014
fix: wildcard expand + group by doesnt skip expand
patricebender Jan 22, 2025
bed7329
Revisit expected queries for $apply
danjoa Jan 28, 2025
f21606a
no special magic for expand + group by
patricebender Feb 3, 2025
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
82 changes: 0 additions & 82 deletions db-service/lib/cqn4sql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,14 +792,6 @@
// this is the alias of the column which holds the correlated subquery
const columnAlias = column.as || ref.map(idOnly).join('_')

// if there is a group by on the main query, all
// columns of the expand must be in the groupBy
if (transformedQuery.SELECT.groupBy) {
const baseRef = column.$refLinks[0].definition.SELECT || ref

return _subqueryForGroupBy(column, baseRef, columnAlias)
}

// 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, inferred.outerQueries)
Expand Down Expand Up @@ -857,80 +849,6 @@
}
return subq
}

/**
* Generates a special subquery for the `expand` of the `column`.
* All columns in the `expand` must be part of the GROUP BY clause of the main query.
* If this is the case, the subqueries columns match the corresponding references of the group by.
* Nested expands are also supported.
*
* @param {Object} column - To expand.
* @param {Array} baseRef - The base reference for the expanded column.
* @param {string} subqueryAlias - The alias of the `expand` subquery column.
* @returns {Object} - The subquery object or null if the expand has a wildcard.
* @throws {Error} - If one of the `ref`s in the `column.expand` is not part of the GROUP BY clause.
*/
function _subqueryForGroupBy(column, baseRef, subqueryAlias) {
const groupByLookup = new Map(
transformedQuery.SELECT.groupBy.map(c => [c.ref && c.ref.map(refWithConditions).join('.'), c]),
)

// to be attached to dummy query
const elements = {}
const wildcardIndex = column.expand.findIndex(e => e === '*')
if (wildcardIndex !== -1) {
// expand with wildcard vanishes as expand is part of the group by (OData $apply + $expand)
return null
}
const expandedColumns = column.expand.flatMap(expand => {
if (!expand.ref) return expand
const fullRef = [...baseRef, ...expand.ref]

if (expand.expand) {
const nested = _subqueryForGroupBy(expand, fullRef, expand.as || expand.ref.map(idOnly).join('_'))
setElementOnColumns(nested, expand.element)
elements[expand.as || expand.ref.map(idOnly).join('_')] = nested
return nested
}

const groupByRef = groupByLookup.get(fullRef.map(refWithConditions).join('.'))
if (!groupByRef) {
throw new Error(
`The expanded column "${fullRef.map(refWithConditions).join('.')}" must be part of the group by clause`,
)
}

const copy = Object.create(groupByRef)
// always alias for this special case, so that they nested element names match the expected result structure
// otherwise we'd get `author { <outer>.author_ID }`, but we need `author { <outer>.author_ID as ID }`
copy.as = expand.as || expand.ref.at(-1)
const tableAlias = getTableAlias(copy)
const res = getFlatColumnsFor(copy, { tableAlias })
res.forEach(c => {
elements[c.as || c.ref.at(-1)] = c.element
})
return res
})

const SELECT = {
from: null,
columns: expandedColumns,
}
return Object.defineProperties(
{},
{
SELECT: {
value: Object.defineProperties(SELECT, {
expand: { value: true, writable: true }, // non-enumerable
one: { value: column.$refLinks.at(-1).definition.is2one, writable: true }, // non-enumerable
}),
enumerable: true,
},
as: { value: subqueryAlias, enumerable: true, writable: true },
elements: { value: elements }, // non-enumerable
},
)
}
}

function getTransformedOrderByGroupBy(columns, inOrderBy = false) {
Expand Down Expand Up @@ -2397,7 +2315,7 @@

const getName = col => col.as || col.ref?.at(-1)
const idOnly = ref => ref.id || ref
const refWithConditions = step => {

Check warning on line 2318 in db-service/lib/cqn4sql.js

View workflow job for this annotation

GitHub Actions / Tests (22) 

'refWithConditions' is assigned a value but never used
let appendix
const { args, where } = step
if (where && args) appendix = JSON.stringify(where) + JSON.stringify(args)
Expand Down
2 changes: 1 addition & 1 deletion db-service/test/cqn4sql/expand.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,7 @@ describe('Unfold expands on associations to special subselects', () => {
})
})

describe('Expands with aggregations are special', () => {
describe.skip('Expands with aggregations are special', () => {
let model
beforeAll(async () => {
cds.model = model = await cds.load(__dirname + '/../bookshop/db/schema').then(cds.linked)
Expand Down
12 changes: 11 additions & 1 deletion test/scenarios/bookshop/read.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ describe('Bookshop - Read', () => {
test('Books with groupby with path expression and expand result', async () => {
const res = await GET(
'/admin/Books?$apply=filter(title%20ne%20%27bar%27)/groupby((author/name),aggregate(price with sum as totalAmount))',
//> SELECT author { name }, sum(price) as totalAmount from Books where title != 'bar' GROUP BY author.name
admin,
)
expect(res.data.value.length).to.be.eq(4) // As there are two books which have the same author
Expand All @@ -50,6 +51,7 @@ describe('Bookshop - Read', () => {
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))',
//> SELECT author { name, ID }, sum(price) as totalAmount from Books where title != 'bar' GROUP BY author.name, author.ID
admin,
)
expect(res.data.value.length).to.be.eq(4) // As there are two books which have the same author
Expand All @@ -66,6 +68,7 @@ describe('Bookshop - Read', () => {
test('same as above, with more depth', async () => {
const res = await GET(
'/admin/Books?$apply=filter(title%20ne%20%27bar%27)/groupby((genre/parent/name),aggregate(price with sum as totalAmount))',
//> SELECT genre { parent { name }}, sum(price) as totalAmount from Books where title != 'bar' GROUP BY genre.parent.name
admin,
)
expect(res.data.value[0].genre.parent.name).to.be.eq('Fiction')
Expand All @@ -74,23 +77,29 @@ describe('Bookshop - Read', () => {
test('pseudo expand using groupby and orderby on same column', async () => {
const res = await GET(
'/admin/Books?$apply=groupby((author/name))&$orderby=author/name',
//> SELECT author { name } from Books GROUP BY author.name ORDER BY author.name
admin,
)
expect(res.data.value.every(row => row.author.name)).to.be.true
})

test('groupby with multiple path expressions', async () => {
const res = await GET('/admin/A?$apply=groupby((toB/toC/ID,toC/toB/ID))', admin)
//> SELECT toB { toC { ID }, toC { toB { ID } from A GROUP BY toB.toC.ID, toC.toB.ID
expect(res.status).to.be.eq(200)
})

test('groupby with multiple path expressions and orderby', async () => {
const res = await GET('/admin/A?$apply=groupby((toB/toC/ID,toB/toC/ID))&$orderby=toB/toC/ID', admin)
//> SELECT toB { toC { ID }, toB { toC { ID } from A GROUP BY toB.toC.ID, toB.toC.ID ORDER BY toB.toC.ID
// REVISIT: This should not work at all, due to conflicting columns !!!
expect(res.status).to.be.eq(200)
})

test('groupby with multiple path expressions and filter', async () => {
const res = await GET('/admin/A?$apply=groupby((toB/toC/ID,toB/toC/ID))&$filter=ID eq 1', admin)
//> SELECT toB { toC { ID },toB { toC { ID } from A WHERE ID = 1 GROUP BY toB.toC.ID, toB.toC.ID
// REVISIT: This should not work at all, due to conflicting columns !!!
expect(res.status).to.be.eq(200)
})

Expand All @@ -108,7 +117,7 @@ describe('Bookshop - Read', () => {
SELECT FROM sap.capire.bookshop.Books as ![FROM]
{
![FROM].title as group,
![FROM].author { name as CONSTRAINT }
![FROM].author { name as CONSTRAINT }
}
where ![FROM].title LIKE '%Wuthering%'
order by group
Expand Down Expand Up @@ -338,6 +347,7 @@ describe('Bookshop - Read', () => {

test('Filter Books(complex filter in apply)', async () => {
const res = await GET(`/browse/Books?$apply=filter(((ID eq 251 or ID eq 252) and ((contains(tolower(descr),tolower('Edgar'))))))`)
//> SELECT from Books where (ID = 251 or ID = 252) and (contains(tolower(descr), tolower('Edgar')))
expect(res.status).to.be.eq(200)
expect(res.data.value.length).to.be.eq(2)
})
Expand Down
6 changes: 6 additions & 0 deletions test/scenarios/sflight/read.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,19 @@ describe('SFlight - Read', () => {
const analyticsPaths = [
/** Requests from the initial page load */
'Bookings?$orderby=FlightDate&$apply=groupby((FlightDate),aggregate(ID with countdistinct as countBookings))&$skip=0&$top=1',
//> SELECT FlightDate, count (distinct ID) as countBookings FROM Bookings GROUP BY FlightDate order by FlightDate limit 1 offset 0
'Bookings?$orderby=countBookings desc&$apply=groupby((status,statusName),aggregate(ID with countdistinct as countBookings))&$skip=0&$top=1',
//> SELECT status, statusName, count (distinct ID) as countBookings FROM Bookings GROUP BY status, statusName order by countBookings desc limit 1 offset 0
'Bookings?$orderby=countBookings desc&$apply=groupby((airline,airlineName),aggregate(ID with countdistinct as countBookings))&$skip=0&$top=1',
//> SELECT airline, airlineName, count (distinct ID) as countBookings FROM Bookings GROUP BY airline, airlineName order by countBookings desc limit 1 offset 0
'Bookings?$apply=aggregate(FlightPrice,CurrencyCode_code)&$filter=FlightPrice ne 0&$skip=0&$top=1',
//> SELECT sum (FlightPrice), sum(CurrencyCode_code) FROM Bookings where FlightPrice != 0 limit 1 offset 0
// REVISIT: works in sflight not in tests
// 'Bookings?$apply=concat(groupby((BookingID,ConnectionID,CurrencyCode_code,FlightDate,ID,TravelID,airline,status))/aggregate($count%20as%20UI5__leaves),aggregate(FlightPrice,CurrencyCode_code),groupby((airline,airlineName),aggregate(FlightPrice,CurrencyCode_code))/concat(aggregate($count%20as%20UI5__count),top(53)))',
'Bookings?$apply=groupby((airline,airlineName),aggregate(FlightPrice with average as avgPrice,FlightPrice with max as maxPrice,FlightPrice with min as minPrice))&$skip=0&$top=1',
//> SELECT airline, airlineName, avg(FlightPrice) as avgPrice, max(FlightPrice) as maxPrice, min(FlightPrice) as minPrice FROM Bookings GROUP BY airline, airlineName limit 1 offset 0
`Bookings?$apply=filter(airline eq 'EA' and status eq 'B')/groupby((ID),aggregate(FlightPrice,CurrencyCode_code))&$count=true&$skip=0&$top=1`
//> SELECT ID, sum (FlightPrice), sum(CurrencyCode_code) FROM Bookings where airline = 'EA' and status = 'B' GROUP BY ID limit 1 offset 0
]

test.each(analyticsPaths)('/analytics/%s', async p => {
Expand Down
Loading