Skip to content

Commit

Permalink
fix: improved = and != with val null for HANA and Postgres (#626)
Browse files Browse the repository at this point in the history
  • Loading branch information
@BobdenOs
BobdenOs authored May 7, 2024
1 parent f4d09e2 commit cbcfe3b
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 67 deletions.
20 changes: 17 additions & 3 deletions db-service/lib/cqn2sql.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -832,18 +832,32 @@ class CQN2SQLRenderer {
operator(x, i, xpr) {

// Translate = to IS NULL for rhs operand being NULL literal
if (x === '=') return xpr[i + 1]?.val === null ? 'is' : '='
if (x === '=') return xpr[i + 1]?.val === null
? _inline_null(xpr[i + 1]) || 'is'
: '='

// Translate == to IS NOT NULL for rhs operand being NULL literal, otherwise ...
// Translate == to IS NOT DISTINCT FROM, unless both operands cannot be NULL
if (x === '==') return xpr[i + 1]?.val === null ? 'is' : _not_null(i - 1) && _not_null(i + 1) ? '=' : this.is_not_distinct_from_
if (x === '==') return xpr[i + 1]?.val === null
? _inline_null(xpr[i + 1]) || 'is'
: _not_null(i - 1) && _not_null(i + 1)
? '='
: this.is_not_distinct_from_

// Translate != to IS NULL for rhs operand being NULL literal, otherwise...
// Translate != to IS DISTINCT FROM, unless both operands cannot be NULL
if (x === '!=') return xpr[i + 1]?.val === null ? 'is not' : _not_null(i - 1) && _not_null(i + 1) ? '<>' : this.is_distinct_from_
if (x === '!=') return xpr[i + 1]?.val === null
? _inline_null(xpr[i + 1]) || 'is not'
: _not_null(i - 1) && _not_null(i + 1)
? '<>'
: this.is_distinct_from_

else return x

function _inline_null(n) {
n.param = false
}

/** Checks if the operand at xpr[i+-1] can be NULL. @returns true if not */
function _not_null(i) {
const operand = xpr[i]
Expand Down
16 changes: 8 additions & 8 deletions db-service/test/cqn2sql/expression.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ describe('expressions', () => {
},
}
const { sql, values } = cqn2sql(cqn)
expect(sql).toMatch(/SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE Foo.x IS \?/i)
expect(values).toEqual([null])
expect(sql).toMatch(/SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE Foo.x IS NULL/i)
expect(values).toEqual([])
})

// We should never have supported that!
Expand All @@ -65,8 +65,8 @@ describe('expressions', () => {
},
}
const { sql, values } = cqn2sql(cqn)
expect(sql).toMatch(/SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE \? IS \?/i)
expect(values).toEqual([null, null])
expect(sql).toMatch(/SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE \? IS NULL/i)
expect(values).toEqual([null])
})

test('ref != null', () => {
Expand All @@ -77,8 +77,8 @@ describe('expressions', () => {
},
}
const { sql, values } = cqn2sql(cqn)
expect(sql).toMatch(/SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE Foo.x IS NOT \?/i)
expect(values).toEqual([null])
expect(sql).toMatch(/SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE Foo.x IS NOT NULL/i)
expect(values).toEqual([])
})

test('val != val', () => {
Expand Down Expand Up @@ -188,8 +188,8 @@ describe('expressions', () => {
}
const { sql, values } = cqn2sql(cqn)
expect({ sql, values }).toEqual({
sql: 'SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE (Foo.x is distinct from ?) or (Foo.x is ?)',
values: [5, null]
sql: 'SELECT Foo.ID,Foo.a,Foo.b,Foo.c,Foo.x FROM Foo as Foo WHERE (Foo.x is distinct from ?) or (Foo.x is NULL)',
values: [5]
})
})

Expand Down
36 changes: 33 additions & 3 deletions hana/lib/HANAService.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -754,14 +754,39 @@ class HANAService extends SQLService {
for (let i = 0; i < xpr.length; i++) {
let x = xpr[i]
if (typeof x === 'string') {
const effective = x === '=' && xpr[i + 1]?.val === null ? '==' : x
// IS (NOT) NULL translation when required
if (x === '=' || x === '!=') {
const left = xpr[i - 1]
const right = xpr[i + 1]
const leftType = left?.element?.type
const rightType = right?.element?.type
// Prevent HANA from throwing and unify nonsense behavior
if (left?.val === null && rightType in lobTypes) {
left.param = false // Force null to be inlined
xpr[i + 1] = { param: false, val: null } // Remove illegal type ref for compare operator
}
if (right?.val === null) {
if (
!leftType || // Literal translation when left hand type is unknown
leftType in lobTypes
) {
xpr[i] = x = x === '=' ? 'IS' : 'IS NOT'
right.param = false // Force null to be inlined
} else {
x = x === '=' ? '==' : '!='
}
}
}

// const effective = x === '=' && xpr[i + 1]?.val === null ? '==' : x
// HANA does not support comparators in all clauses (e.g. SELECT 1>0 FROM DUMMY)
// HANA does not have an 'IS' or 'IS NOT' operator
if (effective in compareOperators) {
if (x in compareOperators) {
endWithCompare = true
const left = xpr[i - 1]
const right = xpr[i + 1]
const ifNull = compareOperators[effective]
const ifNull = compareOperators[x]
x = x === '==' ? '=' : x

const compare = [left, x, right]

Expand Down Expand Up @@ -1260,5 +1285,10 @@ const compareOperators = {
'MEMBER OF': 1,
'LIKE_REGEXPR': 1,
}
const lobTypes = {
'cds.LargeBinary': 1,
'cds.LargeString': 1,
'cds.hana.CLOB': 1,
}

module.exports = HANAService
91 changes: 38 additions & 53 deletions test/scenarios/bookshop/read.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,25 +157,6 @@ describe('Bookshop - Read', () => {
expect(res.data.author.books.length).to.be.eq(2)
})

test('Insert Book', async () => {
const res = await POST(
'/admin/Books',
{
ID: 2,
title: 'Poems : Pocket Poets',
descr:
"The Everyman's Library Pocket Poets hardcover series is popular for its compact size and reasonable price which does not compromise content. Poems: Bronte contains poems that demonstrate a sensibility elemental in its force with an imaginative discipline and flexibility of the highest order. Also included are an Editor's Note and an index of first lines.",
author: { ID: 101 },
genre: { ID: 12 },
stock: 5,
price: '12.05',
currency: { code: 'USD' },
},
admin,
)
expect(res.status).to.be.eq(201)
})

test('Sorting Books', async () => {
const res = await POST(
'/admin/Books',
Expand All @@ -192,19 +173,23 @@ describe('Bookshop - Read', () => {
},
admin,
)
expect(res.status).to.be.eq(201)

const res2 = await GET('/browse/Books?$orderby=title', { headers: { 'accept-language': 'de' } })
expect(res2.status).to.be.eq(200)
expect(res2.data.value[1].title).to.be.eq('dracula')

const q = CQL`SELECT title FROM sap.capire.bookshop.Books ORDER BY title`
const res3 = await cds.run(q)
expect(res3[res3.length - 1].title).to.be.eq('dracula')

q.SELECT.localized = true
const res4 = await cds.run(q)
expect(res4[1].title).to.be.eq('dracula')
try {
expect(res.status).to.be.eq(201)

const res2 = await GET('/browse/Books?$orderby=title', { headers: { 'accept-language': 'de' } })
expect(res2.status).to.be.eq(200)
expect(res2.data.value[1].title).to.be.eq('dracula')

const q = CQL`SELECT title FROM sap.capire.bookshop.Books ORDER BY title`
const res3 = await cds.run(q)
expect(res3[res3.length - 1].title).to.be.eq('dracula')

q.SELECT.localized = true
const res4 = await cds.run(q)
expect(res4[1].title).to.be.eq('dracula')
} finally {
await DELETE('/admin/Books(280)', admin)
}
})

test('Filter Books(multiple functions)', async () => {
Expand All @@ -215,23 +200,28 @@ describe('Bookshop - Read', () => {
expect(res.data.value.length).to.be.eq(3)
})

test.skip('Insert Booky', async () => {
const res = await POST(
'/admin/Booky',
{
ID: 2000,
totle: 'Poems : Pocket Poets',
description:
"The Everyman's Library Pocket Poets hardcover series is popular for its compact size and reasonable price which does not compromise content. Poems: Bronte contains poems that demonstrate a sensibility elemental in its force with an imaginative discipline and flexibility of the highest order. Also included are an Editor's Note and an index of first lines.",
author: { ID: 101 },
genre: { ID: 12 },
stock: 5,
price: '12.05',
currency: { code: 'USD' },
},
test('Filter Books(LargeBinary type)', async () => {
expect(await GET(
`/admin/Books?$filter=image ne null`,
admin,
)
expect(res.status).to.be.eq(201)
)).to.have.nested.property('data.value.length', 0)

expect(await GET(
`/admin/Books?$filter=null ne image`,
admin,
)).to.have.nested.property('data.value.length', 0)


expect(await GET(
`/admin/Books?$filter=image eq null`,
admin,
)).to.have.nested.property('data.value.length', 5)

// intentionally not tranformed `null = image` SQL which always returns `null`
expect(await GET(
`/admin/Books?$filter=null eq image`,
admin,
)).to.have.nested.property('data.value.length', 0)
})

it('joins as subselect are executable', async () => {
Expand Down Expand Up @@ -299,9 +289,4 @@ describe('Bookshop - Read', () => {
return expect(cds.db.run(query)).to.be.rejectedWith(/joins must specify the selected columns/)
})

test('Delete Book', async () => {
const res = await DELETE('/admin/Books(271)', admin)
expect(res.status).to.be.eq(204)
})

})

0 comments on commit cbcfe3b

Please sign in to comment.