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

hakimio commented 1 month ago

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

BobdenOs commented 1 month ago

@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 commented 1 month ago

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?

cap-js / cds-dbs