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

Jump to bottom

fix: improved = and != with val null for HANA and Postgres #626

Merged
merged 3 commits into from
May 7, 2024
Merged

fix: improved = and != with val null for HANA and Postgres #626

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
e6f20cb
fix is null translation for HANA and Postgres
BobdenOs May 6, 2024
03a3402
Update static expectation
BobdenOs May 6, 2024
2956aca
Merge branch 'main' into hana/fix-blob-compare
BobdenOs May 7, 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
20 changes: 17 additions & 3 deletions db-service/lib/cqn2sql.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
})

})