sqlite_fdw icon indicating copy to clipboard operation
sqlite_fdw copied to clipboard

Published 20 hours ago • verified publisher icon pgspider

Implement MAC address support

Open mkgrgis opened this issue 1 year ago • 1 comments

This PR implements PostgreSQL macaddr and macaddr8 columns support as possible mixed affinity column in SQLite. The nearest affinity selected as integer because of there are many fast operations in SQLite including | and &. After this PR sqlite_fdw will can deal with both 12-34-56-78-9A-BC text , 0x0123456789ABC integer and X'0123456789ABC' blob notations. In this PR also:

  • all data type tests moved to subdirectory types instead of extra because of tests of different data types are majority of tests in extra subdirectory.

mkgrgis avatar Jul 15 '24 14:07 mkgrgis

@t-kataym, could you please plan review of this ready PR by pgspider team?

mkgrgis avatar Jul 23 '24 07:07 mkgrgis

All your comments have been checked, @son-phamngoc ! Please ensure now this PR is normal. I'll squash all commits if all is good before merging.

mkgrgis avatar Oct 07 '24 17:10 mkgrgis

@son-phamngoc, look like all of the problems are resolved. Please confirm current implementation and start 3rd round of review.

mkgrgis avatar Oct 14 '24 17:10 mkgrgis

@mkgrgis Please check 2 comments https://github.com/pgspider/sqlite_fdw/pull/101#discussion_r1802238628 and https://github.com/pgspider/sqlite_fdw/pull/101#discussion_r1802241891. I think this PR will be OK for my review after you fixed them.

son-phamngoc avatar Oct 16 '24 02:10 son-phamngoc

Thanks, @son-phamngoc ! All problems are fixed. This PR is ready for squashing and review by @t-kataym .

mkgrgis avatar Oct 16 '24 04:10 mkgrgis

@son-phamngoc , FYI rebased and merged branch macaddr + PostGIS was successfully tested.

mkgrgis avatar Oct 29 '24 09:10 mkgrgis

@son-phamngoc , could you please explain current status of this PR? Is it planned for merging after PostGIS PR?

mkgrgis avatar Nov 29 '24 05:11 mkgrgis

@mkgrgis We are in progress to release new version for supporting PostgreSQL 17. Could you wait for that and then rebase this PR? Then I would like to merge it.

t-kataym avatar Dec 03 '24 02:12 t-kataym

No problems, @t-kataym ! Like most for other my PRs here are no many changes expected.

mkgrgis avatar Dec 03 '24 04:12 mkgrgis

Could you wait for that and then rebase this PR?

@t-kataym , why not inverse order? How do you think, is PostgreSQL 17 support patch for this PR simpler than rebase patch for this PR after PostgreSQL 17 support?

mkgrgis avatar Dec 05 '24 07:12 mkgrgis

@mkgrgis I'm sorry for the inconvenience. It comes from an internal process for release in our company. I appreciate your understanding.

t-kataym avatar Dec 05 '24 08:12 t-kataym

Thanks for clarification, @t-kataym ! I have understood there is private draft branches which have priority for merging during release process.

mkgrgis avatar Dec 05 '24 08:12 mkgrgis

@mkgrgis Thank you for your understanding and waiting for it.
I have modified the code to support PostgreSQL 17(#106).
Could you rebase this PR?

t-kataym avatar Dec 10 '24 04:12 t-kataym

@t-kataym, rebasing is complete. Please review current result or/and discuss previous results of the PR with @son-phamngoc.

mkgrgis avatar Dec 11 '24 11:12 mkgrgis

@mkgrgis Thank you for your effort. @son-phamngoc Could you confirm it again?

t-kataym avatar Dec 12 '24 04:12 t-kataym

@son-phamngoc , look like all problems are resolved. I am ready to merge this PR to GIS branch if this PR will be merged to pgspider's master.

mkgrgis avatar Dec 18 '24 12:12 mkgrgis

@mkgrgis Thanks for your contribution. I have no more comment.
Please wait for @t-kataym's decision.

son-phamngoc avatar Dec 19 '24 01:12 son-phamngoc

Thanks for review, @son-phamngoc !

mkgrgis avatar Dec 19 '24 08:12 mkgrgis

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

t-kataym avatar Dec 20 '24 01:12 t-kataym