Skip to content

Fix CI #114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Apr 11, 2025
Merged

Fix CI #114

merged 17 commits into from
Apr 11, 2025

Conversation

mkgrgis
Copy link
Contributor

@mkgrgis mkgrgis commented Mar 25, 2025

During #109 pre-implementation and discussion was proofed there was no downloaded SQLite version using during test process, but default SQLite library from OS repository.

In this PR:

  • Separate PostGIS downloading script for better private testing on Ubuntu outside of CI.
  • Add SQLITE_FOR_TESTING_DIR variable for location of downloaded and compiled SQLite, which is not OS dependency.
  • Add libsqlite test for showing actual SQLite source code version and commit point.
  • Add sqlite_fdw_sqlite_version() and sqlite_fdw_sqlite_code_source() SQL functions for real called SQLite API version control. Some difference between SQLite C header and real C call is possible in case of wrong compilation process. This is because SQLite C functions, not header constants are used.

@lamdn1409
Copy link
Contributor

@mkgrgis

As we discussed in #109 so far, we expected to separate issues to different/smaller PRs for easy reviewing and merging code.
As my understanding, this PR has some independent issues:

  • Upgrade SQLite version to 3.49.0 for future better JSON transformations support (this issue can be merged to JSON support PR JSON + JSONB support, -> and ->> pushing down for JSON data #109)
  • The downloaded and installed SQLite 3.46.0 is not used by SQLite FDW.
    • Add SQLITE_FOR_TESTING_DIR variable for location of downloaded and compiled SQLite, which is not OS dependency.
    • Add sqlite_fdw_sqlite_version() and sqlite_fdw_sqlite_code_source() SQL functions for real called SQLite API version control. Some difference between SQLite C header and real C call is possible in case of wrong compilation process. This is because SQLite C functions, not header constants are used.
  • Separate PostGIS downloading script for better private testing on Ubuntu outside of CI.
  • Separate testing sequences in different variables instead one long sequence.
  • Add auto_import test for documented but not tested FOREIGN IMPORT SCHEMA behaviour. This test have versions with GIS support and without GIS support.
  • Switch SQLite error codes to more informative modern form instead of legacy codes. Based on https://sqlite.org/rescode.html#extended_result_code_list

Is my understanding correct? Please tell me if I'm wrong.
There is no problem if PR is small, clearer is better.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 26, 2025

Is my understanding correct?

Yes with a small correction.

The downloaded and installed SQLite 3.46.0 is not used by SQLite FDW

The downloaded and installed SQLite 3.46.0 is not used by SQLite FDW CI tests.

@lamdn1409 , I think upgrading SQLite to 3.49.0 can have role of demo-example of non-default SQLite version using in CI testing process. This will also useful for JSON support, but not only. Also it's possible any SQLite version upgrading or downgrading, but I think the example is better because allow don't adopt anything to JSON support.

UPD:

In my opinion this PR have such parts as

  • Non default SQLite version usage: 3.49.0 + SQLITE_FOR_TESTING_DIR variable + sqlite_fdw_sqlite_version() and sqlite_fdw_sqlite_code_source() SQL functions.
  • Add auto_import test for FOREIGN IMPORT SCHEMA behavior.
  • Switch SQLite error codes to more informative modern form instead of legacy codes.

@lamdn1409
Copy link
Contributor

lamdn1409 commented Mar 26, 2025

@mkgrgis

I think upgrading SQLite to 3.49.0 can have role of demo-example of non-default SQLite version using in CI testing process

I know 3.49.0 is good for JSON support. However, please fix the current SQLite 3.46.0 first because we need to make sure all current features of SQLite FDW working well with 3.46.0.

Add auto_import test for FOREIGN IMPORT SCHEMA behavior.

This issue is not related to SQLite 3.46.0 issue. So, please separate it.

Switch SQLite error codes to more informative modern form instead of legacy codes.

Although this change is small, but it is not related to the SQLite 3.46.0 issue, please separate it. I could understand your modification, but please follow the rule strictly, please make PR small and focused and it helps reviewer's jobs easier.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 26, 2025

@lamdn1409 , I'll try to add 2 other PRs. New error codes will be 1st PR, auto_import test will be 2nd. Will this OK?

@lamdn1409
Copy link
Contributor

@mkgrgis

Will this OK?

Yes. It's OK.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 26, 2025

@lamdn1409, let's switch to #115 first. Look like this is good 1st step. I hope review and merge will be fast.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 26, 2025

@lamdn1409 , #116 as overlay of 1st partial PR also is ready. Please review it after first partial PR.

@mkgrgis mkgrgis changed the title Fix CI and refactor tests Fix CI Mar 26, 2025
mkgrgis added 2 commits March 27, 2025 12:11
Co-authored-by: mkgrgis <mihail.aksenov@bk.ru>

Switch to more detailed SQLite error codes

Fix PostGIS text

Errcode reimplement

Add new implementation

Fix PostGIS results

Improve error message according SQLite documentation

Add `auto_import` test for `IMPORT FOREIGN TABLE`
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 27, 2025

@lamdn1409 , let's review the next , #116 PR ?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Apr 5, 2025

@t-hiroshige , please start a review of this PR. Usually there was 1st review, sometimes 2nd control review and 3rd confirm review round directly before merge. In this case @lamdn1409 can be 1st reviewer.

This PR is a pre-condition of yet tested and reviewed about JSON only #109.

My current roadmap about sqlite_fdw:

@lamdn1409
Copy link
Contributor

lamdn1409 commented Apr 9, 2025

@mkgrgis

Before @t-hiroshige reviewing, could you check again my comment at #114 (comment).

I'd like to keep SQLite 3.46.0 to verify all features first. Because you know the system SQLite was used before.
You can upgrade to SQLite 3.49.0 in the PR #109.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Apr 9, 2025

@lamdn1409, no problems. I'll set SQLite 3.46.0 and clean libsqlite test in some minutes.

@lamdn1409
Copy link
Contributor

I'll set SQLite 3.46.0 and clean libsqlite test in some minutes.

I confirmed the result. Please remove this line in the description Upgrade SQLite version to 3.49.0 for future better JSON transformations support.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Apr 9, 2025

Please remove this line in the description Upgrade SQLite version to 3.49.0 for future better JSON transformations support.

Done. This potential can be merged after fixing unsquashed internal commits in mainstream by #116 (comment).

@lamdn1409
Copy link
Contributor

I have no more comments.
@t-hiroshige Could you review this PR?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Apr 10, 2025

@t-hiroshige , also please read #116 (comment) about mainstream commit history before merging.

@t-hiroshige
Copy link
Contributor

@mkgrgis
Sorry for the late response, I've squashed all the #116 commits that were left on master. I think the conflict was caused by that. I'm sorry for the inconvenience, but could you please rebase and resolve it?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Apr 10, 2025

could you please rebase and resolve it?

Done, @t-hiroshige. This was easy. Please continue merge process. This PR also can be squashed during merging to the mainstream.

@t-hiroshige
Copy link
Contributor

@mkgrgis
Thank you for the fix. I will squashed and merge after the check is done

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Apr 10, 2025

@t-hiroshige , the check was done. I'll rebase the next PR about JSON after your merge signal.

@t-hiroshige t-hiroshige merged commit 0c46de4 into pgspider:master Apr 11, 2025
11 checks passed
@mkgrgis mkgrgis deleted the CI_fix branch April 11, 2025 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants