Skip to content

added support for all native user data types PostgreSQL 15 - #193

Open
llucenic wants to merge 35 commits into
denizzzka:masterfrom
llucenic:arinas
Open

llucenic wants to merge 35 commits into
denizzzka:masterfrom
llucenic:arinas

Conversation

@llucenic

Copy link
Copy Markdown
Contributor

Hello, I have added support for all native PostgreSQL user data types (ranges, multiranges, network, bitstrings, geometry, xml and text search vector/query), for now except for multi-dimensional arrays and user-defined types. In order to add support for OidType.PointArray I had to explicitely distinguish between Point[] and Polygon by defining a solid struct Point in dpq2.conv.geometry. Please review and incorporate the changes in the code base on your sole discretion.

Comment thread src/dpq2/conv/net.d Outdated
@denizzzka

denizzzka commented Mar 19, 2023

Copy link
Copy Markdown
Owner

@llucenic Thank you!

In order to add support for OidType.PointArray I had to explicitely distinguish between Point[] and Polygon by defining a solid struct Point in dpq2.conv.geometry

Such case is need to cover by integration test

@llucenic

Copy link
Copy Markdown
Contributor Author

Denis, I do not understand fully your point regarding integration test. Is it possible you kind of fix the tests yourself?

@denizzzka

Copy link
Copy Markdown
Owner

It is common practice to immediately implement a test with a PR, yep? Is there some problem with this?
Tests can be added into native_tests.d file.

@llucenic

Copy link
Copy Markdown
Contributor Author

I have corrected the code, so that it passes at least some of the check jobs. However, the rest of the failures are beyond the scope of my PR.

At the moment, I cannot do much more. Maybe later, and I am not sure though the code I added follows your design of the library. Therefore I have asked you to add the tests yourself while integrating the changes into your code base.

@denizzzka

Copy link
Copy Markdown
Owner

Maybe implementation of ranges as a separate PR would be the best solution. Otherwise this PR is too big.

@denizzzka

denizzzka commented Mar 21, 2023

Copy link
Copy Markdown
Owner

However, the rest of the failures are beyond the scope of my PR.

Yes, looks like this issues itroduced by new compilers and will be solved not in this repository.

But I am worry about lack of integration tests for ranges. This is fundamentally new structures, I would like to cover with tests at least one of each base type.

If it is too difficult feel free to copy and paste here your actual code what works with it and I'll try to implement test by myself

@llucenic

llucenic commented Mar 21, 2023

Copy link
Copy Markdown
Contributor Author

If integration tests for ranges are what makes you integrate this PR in your library, then I think I can provide them. Assuming that we avoid splitting this PR, which does not make much sense to me (as implementation of ranges is only code in conv/ranges.d).

Ľudovít Lučenič added 6 commits May 7, 2024 16:05
Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
…ange), napr. pri načítaní zhora otvoreného časového intervalu, ktorého horná a dolná hranica sú totožné, a nie sú teda Null ani Inf

Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
…a multirozsahov požadované pre prijatie žiadosti o včlenenie kódu do hlavného vývoja knižnice dpq2

Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
@llucenic

Copy link
Copy Markdown
Contributor Author

Hello, I finally got around to fulfill my promise regarding tests for ranges. Actually, I have prepared tests for ranges, range arrays, multiranges and multirange arrays altogether. I hope now (all) my contribution might be merged into main development of your library. Thank you for letting me know.

p.s. the solution at this stage aims solely on reading the data from the database consistently (so convenient writing using some D structs with user-friendly interface is missing); therefore the tests use the simplified (and somehow low-level) byte array constructors; there is this possibility of adding appropriate contructors left open for later updates

@llucenic

Copy link
Copy Markdown
Contributor Author

Regarding the merge conflict, there is only this RecordArray literal to be added from master branch into the pulled version.

@denizzzka

denizzzka commented Jan 29, 2025

Copy link
Copy Markdown
Owner

Regarding the merge conflict, there is only this RecordArray literal to be added from master branch into the pulled version.

I made some minor changes to this PR here: llucenic#1

The only problem is that my PR also contains a merged ~master, so it is difficult to perceive.

My proposal:

  1. You merge current ~master into your PR branch (seems like it is only need merge and for conflict solving just pick oids.d from my PR). This will fix tests and CI running
  2. Then you clearly will see changes proposed by me into your PR and we disscuss it if need
  3. You merge my PR into your branch
  4. Then we need to decissions about my comments to this PR at this page
  5. Then your resulting PR can be squashed into one or maybe 2-4 commits
  6. Merging into ~master

Ľudovít Lučenič added 2 commits January 29, 2025 10:54
…nej vetvy verzie pre platformu Arinas

Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
…ged with v1.2.0)

Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
@llucenic

Copy link
Copy Markdown
Contributor Author

Hi, I followed the steps from your proposal and it seems to be ready now for merge on your side.
Thank you for your code review, your changes are all fine for me.
Kind regards,
Ľudovít

Comment thread src/dpq2/conv/ranges.d Outdated
Comment thread src/dpq2/oids.d
Comment thread src/dpq2/conv/net.d Outdated
Comment thread src/dpq2/conv/bit.d Outdated
Comment thread src/dpq2/conv/geometric.d
Comment thread src/dpq2/conv/native_tests.d
Comment thread src/dpq2/oids.d Outdated
Attempt 2: Tabs converted to spaces, comments for OidType enum added
llucenic and others added 4 commits February 3, 2025 20:41
…adosti o včlenenie kódu do hlavného vývoja knižnice dpq2

Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>
Signed-off-by: Ľudovít Lučenič <ludovit.lucenic@digital-orchestra.sk>

This branch has not been deployed

No deployments
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.

2 participants