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

Restore bit and varbit support with tests #83

Merged
merged 16 commits into from
Nov 16, 2023

Conversation

mkgrgis
Copy link
Contributor

@mkgrgis mkgrgis commented Aug 7, 2023

PostgreSQL bit and varbit dataypes based on SQLite integer affinity, hence maximum length for current implementation is 64 bits. There was untested bit and varbit support in early releases, which was regressed to some fragments of code.

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 comments for your PR. Please help me to check this.

deparse.c Show resolved Hide resolved
expected/12.15/type.out Show resolved Hide resolved
expected/12.15/type.out Outdated Show resolved Hide resolved
expected/12.15/type.out Show resolved Hide resolved
sqlite_fdw.h Outdated Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
@mkgrgis mkgrgis requested a review from nxhai98 November 2, 2023 14:41
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 added some more comment for you modify. Please help me to check it.

expected/16.0/type.out Show resolved Hide resolved
deparse.c Show resolved Hide resolved
@mkgrgis mkgrgis requested a review from nxhai98 November 7, 2023 07:39
sqlite_query.c Outdated Show resolved Hide resolved
@mkgrgis mkgrgis requested a review from t-kataym November 7, 2023 11:07
@nxhai98
Copy link
Contributor

nxhai98 commented Nov 9, 2023

@mkgrgis,
I executed test for your branch and there is a failed test (for all versions):

@@ -1155,7 +1155,7 @@
--- postgresql-12.15/contrib/sqlite_fdw/expected/12.15/type.out
+++ postgresql-12.15/contrib/sqlite_fdw/results/12.15/type.out
 --Testcase 238: very long bit string, expected ERROR, 65 bits
 INSERT INTO "type_VARBIT" ("i", "b") VALUES (14, '01001001010110010100101010001111101101011011011110110001010101010');
 ERROR:  SQLite FDW dosens't support very long bit/varbit data
-HINT:  bit length 65, maxmum 64
+HINT:  bit length 65, maximum 64
 --Testcase 239:

Could you fix this?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Nov 9, 2023

Could you fix this?

Thanks, @nxhai98 ! Fixed in d5e1fea, I am sorry. Look like this PR is ready for merging.

@nxhai98
Copy link
Contributor

nxhai98 commented Nov 9, 2023

Thank you for fixing. I confirmed in my environment. All tests are passed.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Nov 9, 2023

@t-kataym , your opinion?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Nov 15, 2023

@nxhai98 , could you please ask @t-kataym -san about decision about this PR? I want to be in work rhythm with pgspider team but don't understand review plans of the team for PRs from github. For example now I have made two parallel simple PRs with arbitrary review order #89 (testing scripts) and #91 (int overflow). Also I have continued datatype grid normalisation to ISO:SQL in complicated PR #88 which will base for yet drafted and tested "UUID fix" and "float infinity" PRs. Could anyone please give me approximately time milestones?

@t-kataym
Copy link
Contributor

@mkgrgis I'm sorry for late response. I didn't have not enough time.

We confirmed the PR. Is it no problem to merge it into master?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Nov 16, 2023

No problems to merge. Maybe in squashed form if You want, @t-kataym. Thanks.

@t-kataym
Copy link
Contributor

@mkgrgis Thank you. I will merge it.

@t-kataym t-kataym merged commit 91a915f into pgspider:master Nov 16, 2023
@mkgrgis mkgrgis deleted the draft_bit_varbit branch November 16, 2023 10:29
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