Skip to content
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

Refactor data conversion errors and diagnostics, tests, headers and test data directory #91

Merged
merged 19 commits into from
Feb 7, 2024

Conversation

mkgrgis
Copy link
Contributor

@mkgrgis mkgrgis commented Nov 15, 2023

In this PR:

  • PostgreSQL integer-family out of range checks. SQLite int affinity is 1↔1 with PostgreSQL bigint and also have equal ranges. If we have int or smallint in PostgreSQL some input values which is out of range for this data types are possible.
  • sqlite3_column_@(stmt, colid) functions inside of sqlite_convert_to_pg replaced to sqlite3_value_@(val) functions for less parameters and fastest calculations, see sqlite3_value *sqlite3_column_value(sqlite3_stmt *pStmt, int i) and static Mem *columnMem(sqlite3_stmt *pStmt, int i) which will calculated once instead of many sqlite3_column_@(stmt, colid) calls.
  • Detach out_of_range test from int4, add int2 TCs 641c539.
  • In some cases in bigint PostgreSQL column can come a value with real affinity which is out of range. Add PostgreSQL input core function and TCs for this case.
  • Deep refactoring of conversion data error diagnostics to newly implemented ErrorContextCallback function with universal messages about any data conversion problems between PostgreSQL data column and SQLite data value.
  • Detach bitstring test from type test 9bc53a4.
  • Add dtof to float4 PostgreSQL data column branch f0593de (without testing, will tested in other PR about full real affinity support).
  • Move SQLite test data to separate /tmp/sqlite_fdw_test directory for less chaos in /tmp cbba043 . This allow reduce testing time.
  • Refactor headers: remove unnecessary headers, ABC other, limit to some versions some of rare used headers. This allow reduce compile time. e23ba83 and 932b3e0
  • Encapsulate ForeignTable and ForeignServer into ...State structures, because this variables are often calculated near this structures in functions like constructor. f1a1f89

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Dec 27, 2023

@t-kataym, this simple PR is ready for review.

@mkgrgis mkgrgis force-pushed the int_overflow branch 2 times, most recently from 92d3572 to 3835967 Compare December 28, 2023 04:05
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 9, 2024

@t-kataym , could you please write approximate week of review of this PR? Have pgspider team any plans of a new sqlite_fdw release?

Copy link
Contributor

@nxhai98 nxhai98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkgrgis,

Thank for your contribution.
I have some comments for your PR, could you please check it?

sqlite_query.c Outdated Show resolved Hide resolved
sqlite_query.c Show resolved Hide resolved
sql/12.16/extra/int4.sql Outdated Show resolved Hide resolved
sql/12.16/extra/int4.sql Outdated Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 12, 2024

I have some comments for your PR, could you please check it?

All your comments have checked, please start 2nd round of review, @nxhai98 !

@mkgrgis mkgrgis requested a review from nxhai98 January 12, 2024 20:23
@mkgrgis mkgrgis changed the title Int overflow Refactor out of range diagnostics, tests, headers and test data directory Jan 13, 2024
@mkgrgis mkgrgis changed the title Refactor out of range diagnostics, tests, headers and test data directory Refactor data conversion errors and diagnostics, tests, headers and test data directory Jan 13, 2024
Copy link
Contributor

@nxhai98 nxhai98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some more comment, could you please check it?

expected/12.16/extra/bool.out Outdated Show resolved Hide resolved
sqlite_fdw.c Outdated Show resolved Hide resolved
sqlite_query.c Show resolved Hide resolved
expected/12.16/extra/bitstring.out Show resolved Hide resolved
expected/16.0/extra/bool.out Show resolved Hide resolved
sqlite_fdw.c Outdated Show resolved Hide resolved
sqlite_fdw.c Outdated Show resolved Hide resolved
@mkgrgis mkgrgis requested a review from nxhai98 January 15, 2024 08:52
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 16, 2024

All comments are checked and fixed, @nxhai98. Thanks for review! What will be later? 3rd round of your review or review by @t-kataym ?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 22, 2024

Ping, @nxhai98 ! Are there any results of 2 rounds of your review? What will be later?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jan 25, 2024

@t-kataym , could you please write approximate week of review and status of this PR? Will there 3rd review round?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Feb 6, 2024

Hello @t-kataym and @nxhai98 ! Any chances to have feedback or 3rd review round for this PR? What is the status of this PR? Completely PR description after checking all your comments was updated #91 (comment)

@t-kataym
Copy link
Contributor

t-kataym commented Feb 7, 2024

@mkgrgis , Thank you for your contribution.
@nxhai98 , Thank you for your confirmation.
I will merge the PR.

@t-kataym t-kataym merged commit 7dd696c into pgspider:master Feb 7, 2024
@mkgrgis mkgrgis deleted the int_overflow branch February 7, 2024 18:17
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