Skip to content

Commit

Permalink
fix(order by): for localized sorting, prepend table alias (#546)
Browse files Browse the repository at this point in the history
Usually, column references in _simple_ names in order by clauses are
left as is. That means **no table alias** is prepended. In expressions
however, **only source elements** can be reffered to.

That is why in case of localized sorting, we must prepend the table
alias nevertheless, as in the DB specific drivers, a `COLLATE`
expression is wrapped around the string reference. Hence we must replace
the column reference with the underlying source reference.

as reported in #543, postgres is not able to resolve a reference to a
column unambigously if the reference is part of a `COLLATE` expression.
  • Loading branch information
patricebender authored Apr 11, 2024
1 parent 821c1e1 commit a273a92
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 3 deletions.
32 changes: 29 additions & 3 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,7 @@ function cqn4sql(originalQuery, model) {
function getTransformedOrderByGroupBy(columns, inOrderBy = false) {
const res = []
for (let i = 0; i < columns.length; i++) {
const col = columns[i]
let col = columns[i]
if (isCalculatedOnRead(col.$refLinks?.[col.$refLinks.length - 1].definition)) {
const calcElement = resolveCalculatedElement(col, true)
res.push(calcElement)
Expand All @@ -877,8 +877,34 @@ function cqn4sql(originalQuery, model) {
res.push(...getTransformedOrderByGroupBy([dollarSelfReplacement], inOrderBy))
continue
}
const { target } = col.$refLinks[0]
const tableAlias = target.SELECT ? null : getQuerySourceName(col) // do not prepend TA if orderBy column addresses element of query
const { target, definition } = col.$refLinks[0]
let tableAlias = null
if (target.SELECT) {
// usually TA is omitted if order by ref is a column
// if a localized sorting is requested, we add `COLLATE`s
// later on, which transforms the simple name to an expression
// --> in an expression, only source elements can be addressed, hence we must add TA
if (target.SELECT.localized && definition.type === 'cds.String') {
const referredCol = target.SELECT.columns.find(c => {

This comment has been minimized.

Copy link
@johannes-vogel

johannes-vogel Apr 12, 2024

Contributor

What should happen here if no columns are provided (target.SELECT.columns === undefined)?

return c.as === col.ref[0] || c.ref?.at(-1) === col.ref[0]
})
if (referredCol) {
col = referredCol
if (definition.kind === 'element') {
tableAlias = getQuerySourceName(col)
} else {
// we must replace the reference with the underlying expression
const { val, func, args, xpr } = col
if (val) res.push({ val })
if (func) res.push({ func, args })
if (xpr) res.push({ xpr })
continue
}
}
}
} else {
tableAlias = getQuerySourceName(col) // do not prepend TA if orderBy column addresses element of query
}
const leaf = col.$refLinks[col.$refLinks.length - 1].definition
if (leaf.virtual === true) continue // already in getFlatColumnForElement
let baseName
Expand Down
41 changes: 41 additions & 0 deletions db-service/test/cqn4sql/table-alias.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,47 @@ describe('table alias access', () => {
expect(query).to.deep.equal(CQL`SELECT from bookshop.Books as stock { stock.ID } ORDER BY stock.stock`)
})

it('for localized sorting, we must append the table alias for column refs', () => {
// as down the line we always use collation expressions for localized sorting
// we must prepend the table alias.
// The simple reference will be wrapped in the expression and hence, expression name resolution rules kick in
// see also https://github.com/cap-js/cds-dbs/issues/543
const query = SELECT.localized
.from('bookshop.Books')
.columns('title', 'title as foo', 'author.name as author')
.orderBy('title', 'foo')
let res = cqn4sql(query, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(CQL`
SELECT from bookshop.Books as Books
left join bookshop.Authors as author on author.ID = Books.author_ID
{
Books.title,
Books.title as foo,
author.name as author
}
ORDER BY Books.title, Books.title`)
})
it('for localized sorting, replace string expression', () => {
const query = CQL(`SELECT from bookshop.Books {
'simple string' as foo: cds.String,
substring('simple string') as bar: cds.String,
'simple' || 'string' as baz: cds.String,
author.name as author
} order by foo, bar, baz`)
query.SELECT.localized = true
let res = cqn4sql(query, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(CQL`
SELECT from bookshop.Books as Books
left join bookshop.Authors as author on author.ID = Books.author_ID
{
'simple string' as foo: cds.String,
substring('simple string') as bar: cds.String,
'simple' || 'string' as baz: cds.String,
author.name as author
}
ORDER BY 'simple string', substring('simple string'), 'simple' || 'string'`)
})

it('supports ORDER BY clause with expressions', () => {
let query = cqn4sql(
CQL`SELECT from bookshop.Books { ID, ID as stock, ID as x }
Expand Down
1 change: 1 addition & 0 deletions test/scenarios/bookshop/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ require('./insert.test')
require('./delete.test')
require('./update.test')
require('./funcs.test')
require('./orderBy.test')
require('./genres.test')
require('./localization.test')
54 changes: 54 additions & 0 deletions test/scenarios/bookshop/orderBy.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
const cds = require('../../cds.js')
const bookshop = require('path').resolve(__dirname, '../../bookshop')

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

test('collations', async () => {
// the original query is sorted by the **column** "createdBy"
// the resulting query has two query sources in the end --> Authors and Books
// even though both Tables have the element "createdBy", the DB should be able to resolve the
// order by reference to the column unambiguously
const query = CQL(`SELECT from sap.capire.bookshop.Books {
createdBy,
author.name as author
} order by createdBy, author
limit 1`)
const res = await cds.run(query)
expect(res.length).to.be.eq(1)
expect(res[0].author).to.eq('Charlotte Brontë')
})
test('collations with val', async () => {
if(cds.env.requires.db.impl === '@cap-js/hana')
return // FIXME: the `val` is put into window function, which ends in an error on HANA
const query = CQL(`SELECT from sap.capire.bookshop.Books {
'simple string' as foo: cds.String,
author.name as author
} order by foo, author
limit 1`)
query.SELECT.localized = true
const res = await cds.run(query)
expect(res.length).to.be.eq(1)
expect(res[0].author).to.eq('Charlotte Brontë')
})
test('collations with func', async () => {
const query = CQL(`SELECT from sap.capire.bookshop.Books {
concat('simple', 'string') as bar: cds.String,
author.name as author
} order by bar, author
limit 1`)
const res = await cds.run(query)
expect(res.length).to.be.eq(1)
expect(res[0].author).to.eq('Charlotte Brontë')
})
test('collations with xpr', async () => {
const query = CQL(`SELECT from sap.capire.bookshop.Books {
'simple' || 'string' as baz: cds.String,
author.name as author
} order by baz, author
limit 1`)
const res = await cds.run(query)
expect(res.length).to.be.eq(1)
expect(res[0].author).to.eq('Charlotte Brontë')
})
})

0 comments on commit a273a92

Please sign in to comment.