-
Notifications
You must be signed in to change notification settings - Fork 12
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.
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
BobdenOs
wants to merge
3
commits into
main
Choose a base branch
from
fix/improve-sql-simple-queries
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ->'$'`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 herefeatures.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 appliedThere was a problem hiding this comment.
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...There was a problem hiding this comment.
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 returntrue
andfalse
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 patchhdb
forsql_simple_queries:1
and not forsql_simple_queries:2
. This was not done, because @johannes-vogel had concerns about applications that would be usingcds.UInt8
as these would now be returned as Booleans. My opinion on this is that these applications shouldn't usesql_simple_queries
. Or not usehdb
, but using@sap/hana-client
will have a more noticeable impact on the application performance then using the defaultJSON
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 renderingjson
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 tohdb
will most definitely be rejected.There was a problem hiding this comment.
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?