search

pgspider / sqlite_fdw

SQLite Foreign Data Wrapper for PostgreSQL
Other
219 stars 37 forks source link

Full ∞ support for numeric context #97

Closed mkgrgis closed 6 months ago

mkgrgis commented 7 months ago

In this PR:

t-kataym commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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

MinhLA1410 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Thanks @mkgrgis ! So I understand that:

Is that correct?

mkgrgis commented 6 months ago

Is that correct?

Yes, absolutely correct.

MinhLA1410 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

mkgrgis commented 6 months ago

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

MinhLA1410 commented 6 months ago

@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 commented 6 months ago

(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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@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): ...

mkgrgis commented 6 months ago

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

MinhLA1410 commented 6 months ago

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

t-kataym commented 6 months ago

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

t-kataym commented 6 months ago

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

mkgrgis commented 6 months ago

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 commented 5 months ago

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