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: Improve sql_simple_queries flag #960

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+7 −53
Open

fix: Improve sql_simple_queries flag #960

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
ac173c2
Improve sql_simple_queries flag
BobdenOs Dec 19, 2024
3fe50ec
Remove missed variable reference
BobdenOs Dec 19, 2024
955a93a
Merge branch 'main' of https://github.com/cap-js/cds-dbs into fix/imp…
BobdenOs Dec 19, 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
30 changes: 2 additions & 28 deletions db-service/lib/cqn2sql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
const cds = require('@sap/cds')
const cds_infer = require('./infer')
const cqn4sql = require('./cqn4sql')
const _simple_queries = cds.env.features.sql_simple_queries
const _strict_booleans = _simple_queries < 2

const { Readable } = require('stream')

Expand Down Expand Up @@ -277,29 +275,7 @@ class CQN2SQLRenderer {
const SELECT = q.SELECT
if (!SELECT.columns) return sql

const isRoot = SELECT.expand === 'root'
const isSimple = _simple_queries &&
isRoot && // Simple queries are only allowed to have a root
!ObjectKeys(q.elements).some(e =>
_strict_booleans && q.elements[e].type === 'cds.Boolean' || // REVISIT: Booleans require json for sqlite
q.elements[e].isAssociation || // Indicates columns contains an expand
q.elements[e].$assocExpand || // REVISIT: sometimes associations are structs
q.elements[e].items // Array types require to be inlined with a json result
)

let cols = SELECT.columns.map(isSimple
? x => {
const name = this.column_name(x)
const escaped = `${name.replace(/"/g, '""')}`
let col = `${this.output_converter4(x.element, this.quote(name))} AS "${escaped}"`
if (x.SELECT?.count) {
// Return both the sub select and the count for @odata.count
const qc = cds.ql.clone(x, { columns: [{ func: 'count' }], one: 1, limit: 0, orderBy: 0 })
return [col, `${this.expr(qc)} AS "${escaped}@odata.count"`]
}
return col
}
: x => {
let cols = SELECT.columns.map(x => {
const name = this.column_name(x)
const escaped = `${name.replace(/"/g, '""')}`
let col = `'$."${escaped}"',${this.output_converter4(x.element, this.quote(name))}`
Expand All @@ -311,14 +287,12 @@ class CQN2SQLRenderer {
return col
}).flat()

if (isSimple) return `SELECT ${cols} FROM (${sql})`

// Prevent SQLite from hitting function argument limit of 100
let obj = "'{}'"
for (let i = 0; i < cols.length; i += 48) {
obj = `jsonb_insert(${obj},${cols.slice(i, i + 48)})`
}
return `SELECT ${isRoot || SELECT.one ? obj.replace('jsonb', 'json') : `jsonb_group_array(${obj})`} as _json_ FROM (${sql})`
return `SELECT ${SELECT.expand === 'root' || SELECT.one ? obj.replace('jsonb', 'json') : `jsonb_group_array(${obj})`} as _json_ FROM (${sql})`
}

/**
Expand Down
8 changes: 1 addition & 7 deletions hana/lib/HANAService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,19 +394,16 @@ class HANAService extends SQLService {
})
}

let hasBooleans = false
let hasExpands = false
let hasStructures = false
const aliasedOutputColumns = outputColumns.map(c => {
if (c.element?.type === 'cds.Boolean') hasBooleans = true
if (c.elements && c.element?.isAssociation) hasExpands = true
if (c.element?.type in this.BINARY_TYPES || c.elements || c.element?.elements || c.element?.items) hasStructures = true
return c.elements ? c : { __proto__: c, ref: [this.column_name(c)] }
})

const isSimpleQuery = (
cds.env.features.sql_simple_queries &&
(cds.env.features.sql_simple_queries > 1 || !hasBooleans) &&
!hasStructures &&
!parent
)
Expand Down Expand Up @@ -515,7 +512,6 @@ class HANAService extends SQLService {
const blobrefs = []
let expands = {}
let blobs = {}
let hasBooleans = false
let path
let sql = SELECT.columns
.map(
Expand Down Expand Up @@ -613,7 +609,6 @@ class HANAService extends SQLService {
path = this.expr(x)
return false
}
if (x.element?.type === 'cds.Boolean') hasBooleans = true
const converter = x.element?.[this.class._convertOutput] || (e => e)
const sql = x.param !== true && typeof x.val === 'number' ? this.expr({ param: false, __proto__: x }) : this.expr(x)
return `${converter(sql, x.element)} as "${columnName.replace(/"/g, '""')}"`
Expand All @@ -633,7 +628,6 @@ class HANAService extends SQLService {
this.blobs.push(...blobColumns.filter(b => !this.blobs.includes(b)))
if (
cds.env.features.sql_simple_queries &&
(cds.env.features.sql_simple_queries > 1 || !hasBooleans) &&
structures.length + ObjectKeys(expands).length + ObjectKeys(blobs).length === 0 &&
!q?.src?.SELECT?.parent &&
this.temporary.length === 0
Expand Down Expand Up @@ -1195,7 +1189,7 @@ SELECT ${mixing} FROM JSON_TABLE(SRC.JSON, '$' COLUMNS(${extraction})) AS NEW LE

static OutputConverters = {
...super.OutputConverters,
LargeString: cds.env.features.sql_simple_queries > 0 ? e => `TO_NVARCHAR(${e})` : undefined,
LargeString: cds.env.features.sql_simple_queries ? e => `TO_NVARCHAR(${e})` : undefined,
// REVISIT: binaries should use BASE64_ENCODE, but this results in BASE64_ENCODE(BINTONHEX(${e}))
Binary: e => `BINTONHEX(${e})`,
Date: e => `to_char(${e}, 'YYYY-MM-DD')`,
Expand Down
2 changes: 1 addition & 1 deletion hana/lib/drivers/hdb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const iconv = require('iconv-lite')
const { driver, prom, handleLevel } = require('./base')
const { isDynatraceEnabled: dt_sdk_is_present, dynatraceClient: wrap_client } = require('./dynatrace')

if (cds.env.features.sql_simple_queries === 3) {
if (cds.env.features.sql_simple_queries) {
Copy link

@oklemenz2 oklemenz2 Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, let's see:

features.sql_simple_queries: 1: Patch not necessary, as always coming as boolean, right? And is default. Here we don't want to patch hdb here
features.sql_simple_queries: 2: Does not make sense anymore at all, as not needed for HANA (sq3) and accepted for sqlite (sq1)
features.sql_simple_queries: 3: Yes, here it should be applied

Copy link

Choose a reason for hiding this comment

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

Idea is to set sql_simple_queries once, being correct for all databases...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR achieves the same behavior for all databases without having to differ between sql_simple_queries:1|2|3 by having all databases return true and false for Booleans.

While generating the sql_simple_queries:2 SQL statements for HANA and patching the one driver which doesn't have native Boolean support. The original approach was to patch hdb for sql_simple_queries:1 and not for sql_simple_queries:2. This was not done, because @johannes-vogel had concerns about applications that would be using cds.UInt8 as these would now be returned as Booleans. My opinion on this is that these applications shouldn't use sql_simple_queries. Or not use hdb, but using @sap/hana-client will have a more noticeable impact on the application performance then using the default JSON queries. As HANA takes a whole 0.01ms to render a JSON row once the query is JIT compiled.

Copy link

Choose a reason for hiding this comment

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

This PR achieves the same behavior for all databases without having to differ between sql_simple_queries:1|2|3 by having all databases return true and false for Booleans.

Yes, that's good ! But I would not extra patch hdb, when anyways the booleans come correctly from query ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't come correctly from the query when using hdb as the protocol version it uses give the metadata as tinyint not as Boolean. That is why it is being patched instead of rendering json queries.

I tried to find the place where hdb defines the protocol version, but was not successful to change the behavior of HANA to match that for hana-client. A protocol upgrade probably comes with more features then just native Boolean support. So a feature request to hdb will most definitely be rejected.

Copy link

Choose a reason for hiding this comment

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

I thought that boolean comes from JSON function for simple_queries: 1, isn't it?

// Make hdb return true / false
const Reader = require('hdb/lib/protocol/Reader.js')
Reader.prototype._readTinyInt = Reader.prototype.readTinyInt
Expand Down
15 changes: 2 additions & 13 deletions postgres/lib/PostgresService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,22 +397,11 @@ GROUP BY k
}
return col
})
const isRoot = SELECT.expand === 'root'
const isSimple = cds.env.features.sql_simple_queries &&
isRoot && // Simple queries are only allowed to have a root
!Object.keys(q.elements).some(e =>
q.elements[e].isAssociation || // Indicates columns contains an expand
q.elements[e].$assocExpand || // REVISIT: sometimes associations are structs
q.elements[e].items // Array types require to be inlined with a json result
)

const subQuery = `SELECT ${cols} FROM (${sql}) as ${queryAlias}`
if (isSimple) return subQuery

// REVISIT: Remove SELECT ${cols} by adjusting SELECT_columns
let obj = `to_jsonb(${queryAlias}.*)`
return `SELECT ${SELECT.one || isRoot ? obj : `coalesce(jsonb_agg (${obj}),'[]'::jsonb)`
} as _json_ FROM (${subQuery}) as ${queryAlias}`
return `SELECT ${SELECT.one || SELECT.expand === 'root' ? obj : `coalesce(jsonb_agg (${obj}),'[]'::jsonb)`
Copy link

Choose a reason for hiding this comment

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

Cannot judge

} as _json_ FROM (SELECT ${cols} FROM (${sql}) as ${queryAlias}) as ${queryAlias}`
}

doubleQuote(name) {
Expand Down
5 changes: 1 addition & 4 deletions sqlite/lib/SQLiteService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,7 @@ class SQLiteService extends SQLService {
struct: expr => `${expr}->'$'`,
array: expr => `${expr}->'$'`,
// SQLite has no booleans so we need to convert 0 and 1
boolean:
cds.env.features.sql_simple_queries === 2
? undefined
: expr => `CASE ${expr} when 1 then 'true' when 0 then 'false' END ->'$'`,
boolean: expr => `CASE ${expr} when 1 then 'true' when 0 then 'false' END ->'$'`,
Copy link

Choose a reason for hiding this comment

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

OK!

// DateTimes are returned without ms added by InputConverters
DateTime: e => `substr(${e},0,20)||'Z'`,
// Timestamps are returned with ms, as written by InputConverters.
Expand Down
Loading