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.

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

[postgres] sql_simple_queries: 1 + odata_new_adapter: true breaks ieee754compatible: false #860

Open
hakimio opened this issue Oct 23, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@hakimio
Copy link

hakimio commented Oct 23, 2024

Reopening #838 since the original was closed without actually addressing the issue.
Adding TLDR section this time to make sure people understand the issue before closing it.

TLDR

odata_new_adapter: false + sql_simple_queries: 0 + ieee754compatible: false => decimals correctly returned as numbers
odata_new_adapter: false + sql_simple_queries: 1 + ieee754compatible: false => decimals correctly returned as numbers
odata_new_adapter: true + sql_simple_queries: 0 + ieee754compatible: false => decimals correctly returned as numbers
odata_new_adapter: true + sql_simple_queries: 1 + ieee754compatible: false => decimals incorrectly returned as strings


Description of erroneous behaviour

Combination of sql_simple_queries and odata_new_adapter features breaks "ieee754compatible": false setting.

Detailed steps to reproduce

  • Use the following feature toggle combination:
{
    "features": {
        "odata_new_adapter": true,
        "ieee754compatible": false,
        "sql_simple_queries": 1
    }
}

Also, reproducable with "sql_simple_queries": 2

  • Try to make some odata API queries which includes $select, $filter, $apply or pagination parameters.
  • ieee754compatible setting is ignored and decimal/int64 values are returned as strings

Details about your project

Package Version
@cap-js/asyncapi 1.0.2
@cap-js/cds-types 0.6.5
@cap-js/openapi 1.0.6
@cap-js/postgres 1.10.0
@sap/cds 8.3.1
@sap/cds-compiler 5.3.2
@sap/cds-dk 8.3.0
@sap/cds-foss 5.0.1
@sap/cds-mtxs 2.2.0
@sap/eslint-plugin-cds 3.1.0
Node.js v20.13.1
@hakimio hakimio added the bug Something isn't working label Oct 23, 2024
@BobdenOs
Copy link
Contributor

@hakimio I see now that you are talking about @cap-js/postgres specifically. Which makes a lot of sense as the native Postgres driver has determined that int64 and Decimal types can only be properly represented as strings.

Which you can find here: pg -> pg-types -> pg-numeric + pg-int8

The purpose of sql_simple_queries is to lighten the weight on the database by relying on the native driver behavior. After customer concerns about the performance of HANA json rendering features. This concern does not exist for Postgres as the native to_jsonb function is specifically build to be performant (code, docs). Basically with sql_simple_queries on Postgres you are deciding whether you want to use javascript or c to parse your results.

So I would strongly discourage the usage of sql_simple_queries:1/2 when using @cap-js/postgres or @cap-js/sqlite. As these both have very efficient JSON rendering implementations and their drivers are very efficient in how they transfer the JSON results. The Postgres network protocol just sends the length of the json followed by the whole json result which then the pg package puts through JSON.parse. For SQLite the json is put into a Buffer which @cap-js/sqlite puts into JSON.parse which is measurably faster then the driver calling the V8 APIs to create the equivalent objects.

Additionally future features that rely on the JSON result coming from the database directly will not work with sql_simple_queries. For example result set streaming (measurement).

@hakimio
Copy link
Author

hakimio commented Oct 23, 2024

Maybe there should be some warning logged when trying to use sql_simple_queries with postgres or sqlite? Or just make the setting specific for HANA: hana_simple_queries?
How does HANA work with sql_simple_queries? Is there a mechanism which decides how decimals are returned?

@hakimio hakimio changed the title sql_simple_queries: 1 + odata_new_adapter: true breaks ieee754compatible: false [postgres] sql_simple_queries: 1 + odata_new_adapter: true breaks ieee754compatible: false Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants