JSON + JSONB operators `->` and `->>` pushing down implementation in `WHERE` + new SQLite err codes #109

mkgrgis · 2025-01-10T04:04:58Z

In this PR about JSON operators support

Implement pushing down for -> and ->> operators for left both JSON and JSONB operand in WHERE clauses for both string and integer arguments. Based on https://sqlite.org/json1.html
Add types/json test case set showing -> and ->> behaviour for both JSON and JSONB for both SELECT and WHERE usage contexts
Add/restore JSONB missing support in existed code infrastructure
Switch SQLite error codes to more informative modern form instead of legacy codes. Based on https://sqlite.org/rescode.html#extended_result_code_list
Improve CI scripts: separate SQLite version will used for tests, not SQLite version from CI OS.
Full SQLite jsonb support.
Add auto_import test for FOREIGN IMPORT SCHEMA behaviour. This test have versions with GIS support and without GIS support.
Show || and substr pushing down in tests.
Add sqlite_fdw_sqlite_version() and sqlite_fdw_sqlite_code_source() SQL functions.

Motivation: osmium OSM→PostGIS tool generates OSM tags as JSON or JSONB data. Typical usage look like ... WHERE t->>'building' IS NOT NULL or ... WHERE t->>'access' = 'private'.

mkgrgis · 2025-01-10T06:23:54Z

Hello, pgspider team! There is some JSON support problem in CI. Could anyone diagnose?
I have no such problems with OS package

SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> select json_set('[0,1,2]','$[#]','new');
[0,1,2,"new"]

lamdn1409 · 2025-01-23T04:34:03Z

Hello, pgspider team! There is some JSON support problem in CI. Could anyone diagnose? I have no such problems with OS package
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> select json_set('[0,1,2]','$[#]','new');
[0,1,2,"new"]

@mkgrgis

SQLite version on CI is 3.46.0.
The test is OK on my environment (SQLite 3.45.3).
Could you try to change SQLite version on CI?

mkgrgis · 2025-01-26T07:19:51Z

Could you try to change SQLite version on CI?

@lamdn1409 , thanks for proposition, but bc7515d unfortunately gives no result. Maybe we don't know something about compilation in CI environtment.

mkgrgis · 2025-01-28T13:00:42Z

@lamdn1409 , I am still cannot explain the bad result. In the log there is checking whether to support JSON functions... yes line, also default JSON functions status as enabled documented and tested operators listed in documentation.
UPD: Look like Ubuntu have internal SQLite version, because CI can execute some tests without SQLite source code archive downloading. Hence we have version switching problem in case of types/json test.

mkgrgis · 2025-01-29T13:12:58Z

@lamdn1409, in CI was problem of SQLite install: usual library directory is /usr/lib for Ububtu and Debian, but not usual /usr/local/lib. Unfortunately, this change didn't help to fix JSON test results. There are still checking whether to support JSON functions... yes and failed TCs with -> and ->> operators.

mkgrgis · 2025-02-03T04:21:33Z

@lamdn1409 , please look at the state after ef6cae1 : here is no SQLite downloading, bull all tests except json are successfully! Hence SQLite downloading in CI is absolutely not effective for the environment.

mkgrgis · 2025-02-03T04:46:24Z

@lamdn1409 , now there are successfully tests, please review a code.

lamdn1409 · 2025-02-06T02:15:00Z

@mkgrgis sorry for late reply, I had a long vacation.
Glad to know the issue was resolved. Could you tell me what root cause is?

mkgrgis · 2025-02-06T04:29:24Z

@mkgrgis sorry for late reply, I had a long vacation.

I had too :-) My congratulations!

Glad to know the issue was resolved. Could you tell me what root cause is?

CI SQLite compilation script is not effective in complex with FDW compilation. There was wrong --prefix for Ubuntu, but this is not all. Our FDW compilation scripts uses system libsqlite, not downloaded and compiled - this is root cause.
My effective patch is only Ubuntu upgrading, not full FDW compilation normalizing against downloaded SQLite.

mkgrgis · 2025-02-10T04:48:54Z

Ping, @lamdn1409 ?

lamdn1409 · 2025-02-10T10:27:33Z

@mkgrgis Thanks for your information. I understood the root cause. However, I concern about the downloaded SQLite version maybe not used yet. I'm thinking about 02 solutions:

Remove the existing SQLite, then install the downloaded SQLite into the system folder.
Keep the existing SQLite, install the downloaded SQLite into a folder, then build SQLite FDW with that installed SQLite.
How do you think?

mkgrgis · 2025-02-10T13:19:33Z

I prefer the second variant, @lamdn1409:

Keep the existing SQLite, install the downloaded SQLite into a folder, then build SQLite FDW with that installed SQLite.

But I don't think this is a subject of this PR.

lamdn1409 · 2025-02-11T01:47:20Z

@mkgrgis

I prefer the second variant

me, too. However, let me discuss with our members first.

But I don't think this is a subject of this PR.

I think so, we need to separate this issue. Unfortunately, we cannot spend time to solve the issue at the moment. Could you please support to solve it?

mkgrgis · 2025-02-11T10:32:34Z

@lamdn1409 , let's continue review of the code in this PR.

Could you please support to solve it?

No problems. This planned after pure_float fix and schemas support PR, but before returning to RETURNING PR. Will this ok?

I think here is something about PG_LDFLAGS.

lamdn1409 · 2025-02-12T03:23:10Z

No problems. This planned after pure_float fix and schemas support PR, but before returning to RETURNING PR. Will this ok?

Thank you. Your plan is OK.

lamdn1409 · 2025-02-12T03:28:40Z

@mkgrgis

let's continue review of the code in this PR.

I finished reviewing the PR. Please confirm my comments.

mkgrgis · 2025-02-12T04:51:12Z

Please confirm my comments.

Where are this comments @lamdn1409 ? Could you please give URL because I don't see anything here?

sql/13.15/types/json.sql

expected/13.15/types/json.out

mkgrgis · 2025-02-13T09:09:11Z

@lamdn1409 , SQLite compilation autonomy was successfully used in current CI state. Do you think #112 can be closed now?

lamdn1409 · 2025-02-14T09:15:35Z

@lamdn1409 , SQLite compilation autonomy was successfully used in current CI state. Do you think #112 can be closed now?

@mkgrgis Please create a PR for reviewing, it's other issue.

mkgrgis · 2025-02-14T09:19:37Z

@mkgrgis Please create a PR for reviewing, it's other issue.

@lamdn1409 , this problem was detected and resolved in this PR. Currently all tests use not system, but compiled and downloaded SQLite library. You can ensure this. CI Ubuntu is old, but SQLite is more newer.

deparse.c

expected/13.15/types/json.out

lamdn1409 · 2025-02-14T10:39:12Z

@mkgrgis Please create a PR for reviewing, it's other issue.

@lamdn1409 , this problem was detected and resolved in this PR. Currently all tests use not system, but compiled and downloaded SQLite library. You can ensure this. CI Ubuntu is old, but SQLite is more newer.

@mkgrgis It's OK for me. I will review the changes about CI.

GitHubActions/install_sqlite.sh

Makefile

mkgrgis · 2025-02-18T05:51:32Z

Some prepare steps for SQLite jsonb future support.

Could you please explain your implementation?

This is about #109 (comment). In this PR we cannot implement full SQLite jsonb support, but we can: filter WHERE, get or read SELECT, and partially write UPDATE. Unfortunately jsonb INSERT is very hard for this PR.

mkgrgis · 2025-02-18T05:52:07Z

Some prepare steps for SQLite jsonb future support.

Could you please explain your implementation?

This is about #109 (comment). In this PR we cannot implement full SQLite jsonb support, but we can: filter WHERE, get or read SELECT, and partially write UPDATE. Unfortunately jsonb INSERT is very hard for this PR.

mkgrgis · 2025-02-24T04:53:06Z

@lamdn1409 , FYI about SQLite JSONB an SQLite official software engineer Stephan Beal thinks we should use SQL jsonb() only and bind only char* values with text affinity. See https://sqlite.org/forum/forumpost/f958865005

mkgrgis · 2025-02-25T06:59:40Z

@lamdn1409 , full SQLite JSONB support was implemented. I think TC 015 confirm binding without errors for a group of values. The solution is based on in-memory read-only SQLite connection where select jsonb('....') is executed. This is not very elegant solution, but this was recommended by SQLite official software engineer Stephan Beal.

sqlite_query.c

README.md

mkgrgis · 2025-02-26T08:51:07Z

@lamdn1409 , are any problems in PR now?

lamdn1409 · 2025-02-27T02:53:41Z

@lamdn1409 , are any problems in PR now?

@mkgrgis How about this issue #109 (comment)?

mkgrgis · 2025-02-27T05:31:29Z

@mkgrgis How about this issue #109 (comment)?

I need some help with -rpath, @lamdn1409. Could you please get SQLite version after your proposed values of environment variables by #109 (comment) ? In my cases this is OS SQLite version. You can use

Datum
sqlite_fdw_sqlite_version(PG_FUNCTION_ARGS)
{
	PG_RETURN_INT32(SQLITE_VERSION_NUMBER);
}

with

CREATE OR REPLACE FUNCTION sqlite_fdw_sqlite_version()
RETURNS int
AS 'MODULE_PATHNAME'
LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
COMMENT ON FUNCTION sqlite_fdw_sqlite_version()
IS 'Returns used SQLite code version';

lamdn1409 · 2025-02-27T08:49:57Z

@mkgrgis I can get correct SQLite version with this modification on Makefile. Could you try it?

-SHLIB_LINK := -lsqlite3
+SHLIB_LINK := -L/home/vagrant/sqlite3.46/lib -lsqlite3
+PG_CPPFLAGS += -I/home/vagrant/sqlite3.46/include

+SELECT sqlite_version();
+ sqlite_version
+----------------
+        3046000
+(1 row)
+

My system SQLite still be 3.49.0

[vagrant@localhost sqlite_fdw]$ sqlite3
SQLite version 3.49.0 2025-02-06 11:55:18
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite>

Please note that sqlite3 in test.sh should be changed accordingly.

sqlite3 "$testdir/post.db" < sql/init_data/init_post.sql;
sqlite3 "$testdir/core.db" < sql/init_data/init_core.sql;
sqlite3 "$testdir/common.db" < sql/init_data/init.sql;
sqlite3 "$testdir/selectfunc.db" < sql/init_data/init_selectfunc.sql;

mkgrgis · 2025-02-27T08:53:18Z

Thanks, @lamdn1409 , I am beginning this experiment. Please look at my new test libsqlite and 2 new functions. I think this should be included in the PR for CI SQLite version controlling.

mkgrgis · 2025-02-27T10:19:42Z

@lamdn1409 , please look at current results. I think SQLite install process is successfully, we have normal -rpath. Also make info message give us normal -rpath, but tests gives impossible combination of newer SQlite version number and old functionality and code source:

 SELECT sqlite_fdw_sqlite_version();
  sqlite_fdw_sqlite_version 
 ---------------------------
-                   3037002
+                   3049000

but

 SELECT sqlite_fdw_sqlite_code_source();
                             sqlite_fdw_sqlite_code_source                             
 --------------------------------------------------------------------------------------
- 2025-02-06 11:55:18 4a7dd425dc2a0e5082a9049c9b4a9d4f199a71583d014c24b4cfe276c5a77cde
+ 2022-01-06 13:25:41 872ba256cbf61d9290b571c0e6d82a20c224ca3ad82971edc46b29818d5dalt1

and no JSON functions.

Maybe we have different library and header? See https://sqlite.org/c3ref/libversion.html
This document recommends

assert( sqlite3_libversion_number()==SQLITE_VERSION_NUMBER );
assert( strncmp(sqlite3_sourceid(),SQLITE_SOURCE_ID,80)==0 );
assert( strcmp(sqlite3_libversion(),SQLITE_VERSION)==0 );

mkgrgis · 2025-02-27T11:25:10Z

@lamdn1409 , separate SQLite version for testing problem was resolved!
Based on your modifications I have found such Makefile configuration as

ifdef SQLITE_FOR_TESTING_DIR
SHLIB_LINK := -L$(SQLITE_FOR_TESTING_DIR)/lib -lsqlite3
PG_CFLAGS += -I$(SQLITE_FOR_TESTING_DIR)/include -Wl,-rpath,$(SQLITE_FOR_TESTING_DIR)/lib
else
SHLIB_LINK := -lsqlite3
endif

Please note additional -rpath, because otherwise header gives us a new SQLITE_VERSION_NUMBER, but old library by runtime paths gives old sqlite3_libversion_number() C call result.

…ite err codes

sqlite_fdw--1.0--1.1.sql

sqlite_query.c

lamdn1409 · 2025-02-28T02:24:06Z

Please note additional -rpath, because otherwise header gives us a new SQLITE_VERSION_NUMBER, but old library by runtime paths gives old sqlite3_libversion_number() C call result.

It's great the issue was resolved. Congratulations.
I concern whether -L$(SQLITE_FOR_TESTING_DIR)/lib is still necessary?
This change may work?

ifdef SQLITE_FOR_TESTING_DIR
SHLIB_LINK := -Wl,-rpath,$(SQLITE_FOR_TESTING_DIR)/lib -lsqlite3
PG_CFLAGS += -I$(SQLITE_FOR_TESTING_DIR)/include 
else
SHLIB_LINK := -lsqlite3
endif

mkgrgis · 2025-02-28T03:55:25Z

-L$(SQLITE_FOR_TESTING_DIR)/lib is still necessary

This parameter allows us to use (on linking ld step) library functions which is specified for not OS SQLite library version. Without this path we will not use any new C APIs from a new versions, only from OS libsqlite version. High stability of SQLite C APIs gives your illusion about unnecessary -L. How do you think @lamdn1409 ?

lamdn1409 · 2025-02-28T06:48:36Z

This parameter allows us to use (on linking ld step) library functions which is specified for not OS SQLite library version. Without >this path we will not use any new C APIs from a new versions, only from OS libsqlite version

I got it. Thanks for explanation.
I have no more comments for this PR.

mkgrgis · 2025-02-28T06:51:31Z

Thanks, @lamdn1409 for the great work! I was happy to work with you around of this PR. SQLite JSONB was really unexpected for me, but we was able to ensure full support of this data structure in the PR.

mkgrgis · 2025-03-05T08:47:32Z

@t-kataym , could you please give some approximate dates of this PR review? I am having rebased drafts for other PRs, but some little changes after you review are still possible.

mkgrgis mentioned this pull request Jan 10, 2025

RETURNING support #56

Open

mkgrgis force-pushed the json branch from 50c7e80 to 7bfc8e6 Compare February 11, 2025 10:16

mkgrgis mentioned this pull request Feb 11, 2025

Ubuntu CI doesn't use compiled version of downloaded SQLite source code #112

Open

lamdn1409 reviewed Feb 12, 2025

View reviewed changes

mkgrgis requested a review from lamdn1409 February 12, 2025 13:48

mkgrgis changed the title ~~JSON operators pushing down implementation + new SQLite err codes~~ JSON operators -> and ->> pushing down implementation in WHERE + new SQLite err codes Feb 14, 2025

lamdn1409 reviewed Feb 14, 2025

View reviewed changes

deparse.c Outdated Show resolved Hide resolved

deparse.c Outdated Show resolved Hide resolved

expected/13.15/types/json.out Show resolved Hide resolved

lamdn1409 reviewed Feb 14, 2025

View reviewed changes

GitHubActions/install_sqlite.sh Outdated Show resolved Hide resolved

lamdn1409 reviewed Feb 14, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

mkgrgis changed the title ~~JSON operators -> and ->> pushing down implementation in WHERE + new SQLite err codes~~ JSON + JSONB operators -> and ->> pushing down implementation in WHERE + new SQLite err codes Feb 18, 2025

mkgrgis force-pushed the json branch from 8849e06 to 0ede780 Compare February 19, 2025 09:21

mkgrgis force-pushed the json branch from ba06789 to 4389368 Compare February 24, 2025 09:26

lamdn1409 reviewed Feb 26, 2025

View reviewed changes

sqlite_query.c Outdated Show resolved Hide resolved

lamdn1409 reviewed Feb 26, 2025

View reviewed changes

sqlite_query.c Outdated Show resolved Hide resolved

lamdn1409 reviewed Feb 26, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

mkgrgis requested a review from lamdn1409 February 26, 2025 05:18

JSON+JSONB operators -> and ->> pushing down implementation + new SQL…

Loading
Loading status checks…

9455999

…ite err codes

mkgrgis force-pushed the json branch from b6ada81 to 9455999 Compare February 27, 2025 11:59

expand README about separate SQLite version

Loading
Loading status checks…

e6bac9b

lamdn1409 reviewed Feb 28, 2025

View reviewed changes

sqlite_fdw--1.0--1.1.sql Show resolved Hide resolved

lamdn1409 reviewed Feb 28, 2025

View reviewed changes

sqlite_query.c Outdated Show resolved Hide resolved

Refactor to psprinft()

Loading
Loading status checks…

91f634a

mkgrgis requested a review from lamdn1409 February 28, 2025 04:00

JSON + JSONB operators -> and ->> pushing down implementation in WHERE + new SQLite err codes #109

Are you sure you want to change the base?

JSON + JSONB operators -> and ->> pushing down implementation in WHERE + new SQLite err codes #109

Conversation

mkgrgis commented Jan 10, 2025 • edited Loading

mkgrgis commented Jan 10, 2025

lamdn1409 commented Jan 23, 2025 • edited Loading

mkgrgis commented Jan 26, 2025

mkgrgis commented Jan 28, 2025 • edited Loading

mkgrgis commented Jan 29, 2025

mkgrgis commented Feb 3, 2025

mkgrgis commented Feb 3, 2025

lamdn1409 commented Feb 6, 2025

mkgrgis commented Feb 6, 2025

mkgrgis commented Feb 10, 2025

lamdn1409 commented Feb 10, 2025 • edited Loading

mkgrgis commented Feb 10, 2025

lamdn1409 commented Feb 11, 2025 • edited Loading

mkgrgis commented Feb 11, 2025

lamdn1409 commented Feb 12, 2025 • edited Loading

lamdn1409 commented Feb 12, 2025

mkgrgis commented Feb 12, 2025 • edited Loading

mkgrgis commented Feb 13, 2025

lamdn1409 commented Feb 14, 2025

mkgrgis commented Feb 14, 2025

lamdn1409 commented Feb 14, 2025

mkgrgis commented Feb 18, 2025

mkgrgis commented Feb 18, 2025

mkgrgis commented Feb 24, 2025 • edited Loading

mkgrgis commented Feb 25, 2025

mkgrgis commented Feb 26, 2025

lamdn1409 commented Feb 27, 2025

mkgrgis commented Feb 27, 2025 • edited Loading

lamdn1409 commented Feb 27, 2025 • edited Loading

mkgrgis commented Feb 27, 2025

mkgrgis commented Feb 27, 2025 • edited Loading

mkgrgis commented Feb 27, 2025 • edited Loading

lamdn1409 commented Feb 28, 2025

mkgrgis commented Feb 28, 2025

lamdn1409 commented Feb 28, 2025

mkgrgis commented Feb 28, 2025

mkgrgis commented Mar 5, 2025

JSON + JSONB operators `->` and `->>` pushing down implementation in `WHERE` + new SQLite err codes #109

JSON + JSONB operators `->` and `->>` pushing down implementation in `WHERE` + new SQLite err codes #109

mkgrgis commented Jan 10, 2025 •

edited

Loading

lamdn1409 commented Jan 23, 2025 •

edited

Loading

mkgrgis commented Jan 28, 2025 •

edited

Loading

lamdn1409 commented Feb 10, 2025 •

edited

Loading

lamdn1409 commented Feb 11, 2025 •

edited

Loading

lamdn1409 commented Feb 12, 2025 •

edited

Loading

mkgrgis commented Feb 12, 2025 •

edited

Loading

mkgrgis commented Feb 24, 2025 •

edited

Loading

mkgrgis commented Feb 27, 2025 •

edited

Loading

lamdn1409 commented Feb 27, 2025 •

edited

Loading

mkgrgis commented Feb 27, 2025 •

edited

Loading

mkgrgis commented Feb 27, 2025 •

edited

Loading