fix: Improve sql_simple_queries flag
#960
Conversation
BobdenOs
requested review from
patricebender,
gregorwolf,
sebastianesch,
vobu,
danjoa,
johannes-vogel,
stewsk,
sjvans and
David-Kunz
as code owners
| @@ -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) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Idea is to set sql_simple_queries once, being correct for all databases...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I thought that boolean comes from JSON function for simple_queries: 1, isn't it?
| 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 ->'$'`, |
|
|
||
| // 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)` |
sql_simple_queriesshouldn't exist.This change makes it so that only
HANAknows about thesql_simple_queriesflag and it is no longer numeric. It now is eithertruthyorfalsy. There is a gap inhdbwhich uses an older protocol version which usesTINYINTas a fallback for the lack ofBOOLEANsupport when using@sap/hana-clientthis limitation is no longer there.Which means that if applications have columns modeled as
cds.uint8they are not allowed to usesql_simple_queriesandhdbas driver. Either do the default behavior of not usingsql_simple_queriesor use@sap/hana-clientas driver.