JSON + JSONB support, -> and ->> pushing down for JSON data

[mkgrgis]

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
• Full SQLite jsonb support.
• Show || and substr pushing down in tests.

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]

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]

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]

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]

@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]

@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]

@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]

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

[lamdn1409]

@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]

@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]

Ping, @lamdn1409 ?

[lamdn1409]

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

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

[mkgrgis]

I prefer the second variant, @lamdn1409:

2. 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]

@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]

@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]

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]

@mkgrgis

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

I finished reviewing the PR. Please confirm my comments.

[mkgrgis]

Please confirm my comments.

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

[mkgrgis]

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

[lamdn1409]

@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]

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

[lamdn1409]

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

[lamdn1409]

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]

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]

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

[t-kataym]

@mkgrgis I'm sorry for late response.
It seems that your modification in this PS contains an unrelated code and this PR is based on another PR.
So we cannot know the modification for this new feature easily.

If my understanding is correct, I would like to apply one of the following solutions:
a. After the base PR has been accepted, I resume the review of this PR.
b. You remove unnecessary code from this PR.

[mkgrgis]

@mkgrgis I'm sorry for late response. It seems that your modification in this PS contains an unrelated code and this PR is based on another PR.

@t-kataym , what aspect of this PR do you meant as unrelated code? Latest version of PR description was created 2 weeks ago: #109 (comment)

So we cannot know the modification for this new feature easily.

Why? All CI modifications and a new SQL functions are for SQLite autonomous testing only, all macaddr/bool/uuid output modifications and 1 line from connection.c are for new SQLite err codes, all other modification related directly to JSON and JSONb support.

If my understanding is correct, I would like to apply one of the following solutions:
a. After the base PR has been accepted, I resume the review of this PR.

What is the scope of base PR you are meant? Formally this PR was based on some GIS support draft, but GIS data types support was merged and we have direct line now https://github.com/mkgrgis/sqlite_fdw/network .

b. You remove unnecessary code from this PR.

Please give me a description of unnecessary code. Is this about CI modification for testing against non-system SQLite version?

Upd:

remove unnecessary code from this PR

If you are about auto_import test, this test shows SQLite formal data type translation to PostgreSQL data type, because documented rules was absolutely untested including new rules for JSON and JSONb.

[t-kataym]

@mkgrgis
For example, you modified .github/workflows/CI.yml.
Could you tell me why this modification is related to JSON operators support?

[mkgrgis]

Could you tell me why this modification is related to JSON operators support?

Yes. This is result of wrong previous CI process, which didn't use special downloaded SQLite version for testing, but system version. Here was a long discussion after #109 (comment) resolved with effective solution at #109 (comment) . Hence we was unable to test against new SQLite version where there is better JSON support.
Also downloaded SQLite version was used as trivial dependency for GIS libraries during GIS support testing. Here is the main reason of CI.yml reordering and refactoring.

[t-kataym]

Sorry, I still don't understand the relationship between the modification of CI and the feature of JSON operators support. If they are independent issues/features, it is better to separate them into different PRs.

[mkgrgis]

@t-kataym , PR with some JSON support cannot have successfully json test because the fresh version of SQLite cannot be used for testing. Hence CI refactoring is needed for testing against new SQLite version. Does you want to tell me about separating CI changes to previous formal PR without any JSON support context?

[mkgrgis]

@t-kataym , there is such logic of PR scope expanding as: JSON implementation ➛ @lamdn1409 's note about SQLite JSONB ➛ SQLite JSONB implementation ➛ SQLite 3.49.0 only stable JSON support with negative indexes of arrays like in PostgreSQL ➛ ensure CI SQLite downloaded version is not effective for testing, but OS SQLite version ➛ refactoring of CI for using downloaded SQLite version including -rpath settings ➛ add PostgreSQL SQL function to check and ensure SQLite code version and source repository point ➛ add simple test for SQLite code source ➛ add test to transformation SQLite formal column type to PostgreSQL DDL during import, because documented but not tested.

[t-kataym]

The use of unintentional SQLite version is a general issue of CI environment on this repository.
The fix of this issue is not for only JSON support feature.
Could you create a new PR for fixing CI environment issue?
I will accept it, then could you rebase this PR?

[mkgrgis]

@t-kataym , CI, testing and error codes part of this PR was successfully separated in #114. Please begin a discussion about content of that PR.

[t-kataym]

@mkgrgis Thank you for your understanding and separating the PR. We will check it.

[mkgrgis]

@t-kataym , scope of this PR was successfully fragmented to simple partial step-by-step accumulated PRs:

• 1st Support extended result codes that provide more detailed information about errors #115,
• 2nd Add auto_import test for IMPORT FOREIGN TABLE and test sequence refactoring #116,
• 3rd Fix CI #114

before this complex PR. All PRs are ready for review.

[mkgrgis]

@t-hiroshige , @lamdn1409 this PR is ready for 2nd review round, all base PRs was merged, all tests are successfully.

[lamdn1409]

@mkgrgis I have no more comments.
@t-hiroshige Please help review this PR.

[t-hiroshige]

@mkgrgis Thank you for creating the PR.
@lamdn1409 Thank you for your review.
I accept it.