Full ∞ support for numeric context

[mkgrgis]

In this PR:

• Deparsing of PostgreSQL ∞ special numeric values as 9e999 for SQLite
• Add mixed affinity data normalization function for SQLite for reading case insensitive signed or not values such as +Infinity, -inf, Inf etc.
• Add tests for correct arithmetical ordering.
• Change deparsing of float/numeric values in many tests from direct reading to data normalization function.
• Add some inactive tests for NaN values, add detailed comments about current processing of NaN values, but no changes to current NaN processing. Discussing about NaN will be continues in https://github.com/pgspider/sqlite_fdw/issues/36.
• Unify aggregates test with version of test file for PostgreSQL 16.0.

[t-kataym]

@mkgrgis I'm sorry for the late reply.
Test on PostgreSQL 13 failed and the PR #78 was merged. Could you confirm it and rebase to the latest code?

[mkgrgis]

@t-kataym , I have updated this branch and resolved file conflict. Thank for #78 merging!

PostgreSQL 13 testing problems is specific for Ubuntu and if I'll correct .out file tests for Fedora/RHEL will not passed. Should I treat Ubuntu only as true testing environment? In some of my PRs I was corrected this lines, but someone from pgspider team tell me this is not necessary.

Update: Pg13 testing problem was fixed anyway in https://github.com/pgspider/sqlite_fdw/pull/97/commits/c293164c0ae1c5f867d37416a39b648a75e0f04d, but problems during Pg13 RHEL/Fedora testing are still possible.

[t-kataym]

@mkgrgis Thank you for the update. We will confirm it.

[MinhLA1410]

Hello @mkgrgis, thanks for your hard work. I have question about this description. Could you help me to answer them?

Add tests for correct arithmetical ordering

I don't understand your purpose of this description. Does the modification relate test file extra/aggregates.sql ? You added testcases from version 16.0 sql/16.0/extra/aggregates.sql to older versions, right? Could you tell me the purpose of this?

[mkgrgis]

Hello, @MinhLA1410 !

Add tests for correct arithmetical ordering

I don't understand your purpose of this description.

In PostgreSQL ±∞ values sorted before or after any usual numeric values according ISO:SQL, in SQLite infinity values with text affinity doesn't sorted in proper ISO:SQL order, here is my added tests of the implementation.

Does the modification relate test file extra/aggregates.sql ?

No. This test was only unified with the file from 16.0 version.

You added testcases from version 16.0 sql/16.0/extra/aggregates.sql to older versions, right?

Yes. I meant no other changes to aggregates test.

[MinhLA1410]

Thanks @mkgrgis ! So I understand that:

• Add tests for correct arithmetical ordering just is related to the modification in https://github.com/mkgrgis/sqlite_fdw/blob/cbde7e860bd2a47c2cdee54256be789a13fbb7c0/expected/16.0/extra/float8.out#L2029
• About the modification of aggregates test is only unified with the file from 16.0 version.

Is that correct?

[mkgrgis]

Is that correct?

Yes, absolutely correct.

[MinhLA1410]

Yes, absolutely correct.

Thanks @mkgrgis ,

Add some inactive tests for NaN values, add detailed comments about current processing of NaN values, but no changes to current NaN processing. Discussing about NaN will be continues in https://github.com/pgspider/sqlite_fdw/issues/36.

This specification is not yet clear and is under ongoing discussion. Most of the source code test code related to it has been commented out. So could you separate them into another PR? (We would like to keep the code in master branch clean, with only official code (and comment) that has been tested and verified.)

[mkgrgis]

Most of the source code test code related to it has been commented out. So could you separate them into another PR?

Will the separate PR "Add comments about NaN" correct in this repository? This comments explains most problematic NaN processing, but doesn't change any behaviour. Main purpose of the comments - preparing to discussion https://github.com/pgspider/sqlite_fdw/issues/36 This discussion will be hard, because according ISO:SQL a value must be readable (SELECT), writeable (INSERT, UPDATE) and detectable or filtrable (WHERE = < >). In case of NaN we can select only some 2 properties from 3. I don't know which will be the selected properties for NaN value, but the comments allow to see all alternatives. During preparing this PR I meant NaN is the same special float-like value as ∞ values, but only commented here, not implemented.

We would like to keep the code in master branch clean, with only official code (and comment) that has been tested and verified.

NaN processing is not well tested now, but I think my comments makes current NaN processing more verifiable for future tests, also commented in my PR.

[MinhLA1410]

I understand it.

because according ISO:SQL a value must be readable (SELECT), writeable (INSERT, UPDATE) and detectable or filtrable (WHERE = < >).

NaN is the same special float-like value as ∞ values, but only commented here, not implemented.

But as your comment above, the read/write/filter capability of NaN is not yet clearly defined. Almost sources are just a comment, not implement official (Ex: https://github.com/pgspider/sqlite_fdw/pull/97/files#diff-ffa2eb209af1443703d26c7ba6c34a71b107f39b07568963f97aa36d0aeaaad3R1064, https://github.com/pgspider/sqlite_fdw/pull/97/files#diff-9d9a528d30cdf6046c5a7aafa87ab9b725ce0659fed88cf8fd82b5d3cb00c126R241, https://github.com/pgspider/sqlite_fdw/pull/97/files#diff-2e410e26fd80d47822bdeb9a676e5cc070f13073602d053db19bb87f799dbd8fR400)

We cannot accept these now. We don't accept TODO comments in the source code, only official code + comments can merge to the master I think we should focus to ∞ support for numeric context The discussion for NaN should be in #36 and the implement for it should be in the future (after discussion of #36 is done)

[mkgrgis]

Ok, @MinhLA1410 . I'll remove this TCs and code fragments.

[mkgrgis]

@MinhLA1410 , will https://github.com/pgspider/sqlite_fdw/pull/97/commits/1db7f67501347ccecec093eb881ded5ccc0b4581 enough?

[MinhLA1410]

@mkgrgis ,

will https://github.com/pgspider/sqlite_fdw/commit/1db7f67501347ccecec093eb881ded5ccc0b4581 enough?

It's OK. Thank you.

In PostgreSQL ±∞ values sorted before or after any usual numeric values according ISO:SQL, in SQLite infinity values with text affinity doesn't sorted in proper ISO:SQL order, here is my added tests of the implementation.

I would like to confirm this point again. As you said, I can understand that Before your implementation, sqlite_fdw cannot sorted values correctly (mean -infinity < - 1 < 0 < 1 < +infinity), after your implementation sqlite_fdw can sorted values correctly. Because I tested with sqlite_fdw master branch, the results are sorted correctly (even on sqlite server)

-- On postgres
postgres=# select * from "type_FLOAT_INF" order by f asc,i;
 i |     f     
---+-----------
 1 | -Infinity
 3 | -Infinity
 5 |   -1e+308
 6 |         0
 7 |    1e+308
 2 |  Infinity
 4 |  Infinity
(7 rows)

-- On sqlite
sqlite> select * from "type_FLOAT_INF" order by f asc,i;
1|-Inf
3|-Inf
5|-1.0e+308
6|0.0
7|1.0e+308
2|Inf
4|Inf

If I change column f of table "type_FLOAT_INF" to text type (case text affinity) and use your branch to sort. the results on postgres are same on sqlite server.

-- On sqlite server
sqlite> create table "type_FLOAT_INF"  (i int primary key, f text);
sqlite> INSERT INTO  "type_FLOAT_INF" VALUES (1, -1e999),(2, 1e999),(3, -9e999),(4, 9e999),(5,-1e308),(6, 0),(7, 1e308);
sqlite> select * from  a order by f asc, i;
5|-1.0e+308
1|-Inf
3|-Inf
6|0
7|1.0e+308
2|Inf
4|Inf

-- On postgres 
postgres=# select * from "type_FLOAT_INF"  order by f asc, i;
 i |     f     
---+-----------
 5 | -1.0e+308
 1 | -Inf
 3 | -Inf
 6 | 0
 7 | 1.0e+308
 2 | Inf
 4 | Inf
(7 rows)

I don't see the match between current behavior with your description above. Could you explain it more? Have I misunderstood something?

[mkgrgis]

(mean -infinity < - 1 < 0 < 1 < +infinity), after your implementation sqlite_fdw can sorted values correctly. Because I tested with sqlite_fdw master branch, the results are sorted correctly

This is for case where all values have real affinity. But if some of ∞ value forms, for example -inf and +Infinity will have text affinity there will neither correct sorting -infinity < -1 < 0 < 1 < +infinity nor arithmetic context in SQLite. For PostgreSQL correct sorting in case of such text input is not problem, because it's ISO:SQL RDBMs, but for SQLite is. Please refer TC with mixed affinity of ∞ values where i=17..21 gives different text forms. Also there are many tests for ∞ values with text affinity around of this TC.

[MinhLA1410]

@mkgrgis ,

sqlite> insert into "type_FLOAT_INF" values (10, '-Inf');
sqlite> insert into "type_FLOAT_INF" values (11, '+Infinity');

sqlite> select i,typeof(f) from "type_FLOAT_INF";
1|real
2|real
3|real
4|real
5|real
6|real
7|real
10|text
11|text
sqlite> select * from "type_FLOAT_INF";
1|-Inf
2|Inf
3|-Inf
4|Inf
5|-1.0e+308
6|0.0
7|1.0e+308
10|-inf
11|+Infinity
sqlite> select * from "type_FLOAT_INF" where f < '+Inf';
1|-Inf
2|Inf
3|-Inf
4|Inf
5|-1.0e+308
6|0.0
7|1.0e+308
sqlite> select * from "type_FLOAT_INF" where f < 'Inf';
1|-Inf
2|Inf
3|-Inf
4|Inf
5|-1.0e+308
6|0.0
7|1.0e+308
10|-inf
11|+Infinity

If the table mixed real affinity and text affinity of ∞ value forms. SQLite cannot sort correctly in arithmetic context.
Your PR makes the sorting correct with arithmetic order on Postgres, Is it correct?

[mkgrgis]

Your PR makes the sorting correct with arithmetic order on Postgres, Is it correct?

Yes. Because in PostgreSQL there is 2 possible methods of ∞ value input: text constant like -Infinify or special float value, but SQLite supports only overflow deparsing as ∞ value.

[mkgrgis]

@MinhLA1410 , just for info around of this PR and about different ∞ processing between PostgreSQL and SQLite . From https://www.sqlite.org/releaselog/3_45_3.html

Changes in this specific patch release, version 3.45.3 (2024-04-15): ...

• Fix a bug in sum() that could cause it to return NULL when it should return Infinity. Forum post 23b8688ef4.

[mkgrgis]

All review rounds are completed, all comments are checked, @MinhLA1410. Does this means the next round will de done by @t-kataym ?

[MinhLA1410]

@mkgrgis Yes. I will leave the decision to @t-kataym

[t-kataym]

@MinhLA1410 Thank you for reviewing.
@mkgrgis Thank you for fixing. I will confirm and merge it if no problem.

[t-kataym]

@mkgrgis Thank you for your contribution. This PR was merged.

[mkgrgis]

Thanks, to pgspider team, @t-kataym ! Now I can prepare PR about NaN after some decision around of https://github.com/pgspider/sqlite_fdw/issues/36#issuecomment-2117350187 or I need help with testing environment for https://github.com/pgspider/sqlite_fdw/pull/96 , because I have no problem with compile environment. Which of this tasks have higher priority for pgspider team?

[mkgrgis]

@MinhLA1410, could you please ask @t-kataym about PR around of NaN support? I think this is short and easy PR.