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

Add mixed affinity UPDATE, unify UUID behaviour #95

Merged
merged 7 commits into from
Mar 29, 2024

Conversation

mkgrgis
Copy link
Contributor

@mkgrgis mkgrgis commented Dec 27, 2023

In this PR

  • UPDATE works with column_type option for mixed affinity cases and writes different affinity values depends on value of the option. This fix Column option column_type doesn't works with UPDATE #90. This issue affects uuid and timestamp types of data, implemented in different time. Mixed affinity timestamp was implemented before uuid support in Add uuid support #82,
  • UUID behaviour and tests are unified between all supported PostgreSQL versions for full mixed affinity support with both of SELECT/WHERE usage, INSERT and UPDATE,
  • regress tests reordered: sqlite_fdw_post firsts, than tests about data types in ABC order, other sequence with no changes,
  • add a function which converted column_type option value to SQLite affinity code for standard values with int as integer affinity synonym,
  • uuid test renumbering and add COSTS OFF in some TCs,
  • add to uuid test important TC about semantics conflict in SQLite: the same UUID values with different affinity, but no error during SQLite unique check,
  • add current behaviour for Infinity values in floatN column into type tests with SQLite unique value check. No code changes. This behaviour should be tested before special planned PR about special values in floatN columns,
  • add translation rule for IMPORT FOREIGN SCHEMA if SQLite formal column name is uuid.

@mkgrgis mkgrgis force-pushed the unify_uuid_and_add_bitsring_test branch from ef32b70 to a8a8c06 Compare December 28, 2023 18:30
@mkgrgis mkgrgis force-pushed the unify_uuid_and_add_bitsring_test branch 4 times, most recently from e2dcaa7 to 5f66998 Compare February 7, 2024 19:32
@mkgrgis mkgrgis changed the title Add mixed affinity UPDATE, detach bitstring tests Add mixed affinity UPDATE, unify UUID behaviour Feb 8, 2024
@mkgrgis mkgrgis force-pushed the unify_uuid_and_add_bitsring_test branch from 0642111 to a8ba629 Compare February 8, 2024 03:38
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 6, 2024

@t-kataym , could you please write me approximate date of review of this PR?

@t-kataym
Copy link
Contributor

@mkgrgis , Thank you for your contribution. We would like to confirm the PR and then merge it in this month if no problem.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 14, 2024

Thanks, @t-kataym ! In this case I'll also prepare PR with macaddr and macaddr8 support to the end of April.

@son-phamngoc
Copy link

Hello @mkgrgis , thanks for your hard work.
I have 2 questions about this description. Could you help me to answer them?

add translation rule for IMPORT FOREIGN SCHEMA if SQLite formal column name is uuid.

I see that you created a custom function to convert text/blob to uuid, and deparsed remote UPDATE query to call it. Therefore, I think this change should be for UPDATE, not IMPORT FOREIGN SCHEMA. Is that correct?

special_affinity = (pg_attyp == UUIDOID && preferred_affinity == SQLITE3_TEXT) ||
                   (pg_attyp == TIMESTAMPOID && preferred_affinity == SQLITE_INTEGER);
if (special_affinity)
{
    ....

I see that you handled 2 cases here: uuid and timestamp, but your description in this PR only mentioned about uuid case.
Could you add more description for timestamp case?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 19, 2024

Thanks, @son-phamngoc ! Added in description

fix #90. This issue affects uuid and timestamp types of data, implemented in different time. Mixed affinity timestamp was implemented before uuid support in #82,

The second

I see that you created a custom function to convert text/blob to uuid, and deparsed remote UPDATE query to call it. Therefore, I think this change should be for UPDATE, not IMPORT FOREIGN SCHEMA. Is that correct?

Yes. Rule for IMPORT FOREIGN SCHEMA is outside of deparsing. It's only about better table auto DDL for PostgreSQL. In SQLite was no strict column data types or affinity before special CREATE TABLE ...STRICT mode.

@son-phamngoc
Copy link

Yes. Rule for IMPORT FOREIGN SCHEMA is outside of deparsing. It's only about better table auto DDL for PostgreSQL. In SQLite was no strict column data types or affinity before special CREATE TABLE ...STRICT mode.

@mkgrgis So, I think the description should be "add translation rule for UPDATE if SQLite formal column name is uuid.". Could you update it?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 20, 2024

So, I think the description should be "add translation rule for UPDATE if SQLite formal column name is uuid

No, @son-phamngoc. In SQLite data processing code there is a common good style not to use formal column definitions at all, only affinity de facto of a SQLite data value. UPDATE, SELECT, any WHERE etc. works only based on affinity.
In this FDW common code style is not applicable only to a function for IMPORT FOREIGN SCHEMA. In my PR there is no formal column definition usage outside of the function. Why? Because IMPORT FOREIGN SCHEMA gives recommended data types for PostgreSQL based on SQLite formal column definitions. FDW thinks the result will be normal, but is ready to any affinity according https://github.com/pgspider/sqlite_fdw/tree/master?tab=readme-ov-file#datatypes .

@son-phamngoc
Copy link

@mkgrgis I thought that description related to your modification in sqlite_deparse_direct_update_sql function of deparse.c.
But according to your explanation, it seems it is different.
Could you point me out which parts of your modification related to that description? I do not see any code change or test case related to IMPORT FOREIGN SCHEMA in this PR.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 20, 2024

Could you point me out which parts of your modification related to that description?

I am sorry, @son-phamngoc. There was no that modification. I was sure this was committed by me, but before 9c81b0f this was only in my private draft. Now this is fixed.

Copy link

@son-phamngoc son-phamngoc left a comment

Choose a reason for hiding this comment

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

@mkgrgis Thanks for your support.
I reviewed you PR. Could you check my comments?

README.md Outdated Show resolved Hide resolved
deparse.c Outdated Show resolved Hide resolved
deparse.c Show resolved Hide resolved
deparse.c Outdated Show resolved Hide resolved
sqlite_fdw.c Outdated Show resolved Hide resolved
sqlite_data_norm.c Outdated Show resolved Hide resolved
sqlite_data_norm.c Outdated Show resolved Hide resolved
sqlite_fdw.c Outdated Show resolved Hide resolved
deparse.c Outdated Show resolved Hide resolved
expected/16.0/extra/uuid.out Outdated Show resolved Hide resolved
@mkgrgis mkgrgis requested a review from son-phamngoc March 26, 2024 09:40
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 28, 2024

I reviewed you PR. Could you check my comments?

All your comments are checked, @son-phamngoc ! Let's request the second review?

@son-phamngoc
Copy link

@mkgrgis Thanks for your support.
I executed test for this PR in my environment and they all passed.
I see no problem in this PR for now.
I will leave the decision to @t-kataym.

@t-kataym
Copy link
Contributor

@son-phamngoc Thank you for your confirmation.
@mkgrgis Thank you for your contribution.
I will merge the PR.

@t-kataym t-kataym merged commit a272452 into pgspider:master Mar 29, 2024
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 29, 2024

Thanks, @t-kataym! My next PR from #66 series will be about ∞ in float and numeric data, the branch is almost ready.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Mar 29, 2024

Mixed affinity support PRs continued with #97.

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.

Column option column_type doesn't works with UPDATE
3 participants