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

Implement MAC address support #101

Merged
merged 18 commits into from
Dec 20, 2024
Merged

Implement MAC address support #101

merged 18 commits into from
Dec 20, 2024

Conversation

mkgrgis
Copy link
Contributor

@mkgrgis mkgrgis commented Jul 15, 2024

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.
  • sed from test.sh changed to environment variable REGRESS to deal with partial or non-default tests. This allows define REGRESS and do not touch Makefile content. In case of void REGRESS variable default list of tests is applied.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Jul 23, 2024

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

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 Thank you very much for your hard work.
I have some comment for this PR. Please help me to check them.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
expected/12.16/types/macaddr6.out Outdated Show resolved Hide resolved
deparse.c Outdated Show resolved Hide resolved
deparse.c Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
sqlite_query.c Show resolved Hide resolved
sqlite_query.c Outdated Show resolved Hide resolved
expected/12.16/types/macaddr6.out Outdated Show resolved Hide resolved
expected/16.0/types/macaddr8.out Outdated Show resolved Hide resolved
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Oct 7, 2024

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 mkgrgis requested a review from son-phamngoc October 7, 2024 17:15
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Oct 14, 2024

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

@son-phamngoc
Copy link

@mkgrgis Please check 2 comments #101 (comment) and #101 (comment). I think this PR will be OK for my review after you fixed them.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Oct 16, 2024

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

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Oct 29, 2024

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

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Nov 29, 2024

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

@mkgrgis mkgrgis mentioned this pull request Dec 2, 2024
@t-kataym
Copy link
Contributor

t-kataym commented Dec 3, 2024

@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.

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Dec 3, 2024

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

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Dec 5, 2024

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?

@t-kataym
Copy link
Contributor

t-kataym commented Dec 5, 2024

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

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Dec 5, 2024

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

@t-kataym
Copy link
Contributor

@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?

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Dec 11, 2024

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

@t-kataym
Copy link
Contributor

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

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 hard work.
I have some comments. Please help me to check them.

Makefile Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mkgrgis mkgrgis requested a review from son-phamngoc December 13, 2024 07:30
@mkgrgis
Copy link
Contributor Author

mkgrgis commented Dec 18, 2024

@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.

@son-phamngoc
Copy link

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

@mkgrgis
Copy link
Contributor Author

mkgrgis commented Dec 19, 2024

Thanks for review, @son-phamngoc !

@t-kataym
Copy link
Contributor

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

@t-kataym t-kataym merged commit af42d07 into pgspider:master Dec 20, 2024
11 checks passed
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