search

pramsey / pgsql-ogr-fdw

PostgreSQL foreign data wrapper for OGR
MIT License
237 stars 34 forks source link

ogr_fdw_info Enhancements #183

Closed mccuskk closed 3 years ago

mccuskk commented 4 years ago

Hi

Thanks for looking at the patch. I've reformatted it to reduce the noise (sorry, it was based on an earlier version before you ran astyle on it) and reworked it a little. I have added some more explanation below

We use ogr_fdw_info as part of an automated process for loading client spreadsheets (XLSX) but the current version was not quite suitable for the following reasons.

Unable to specify output server name. Unable to specify output table name. Unable to use ogr layer index rather than layer. Unable to add config options to the foreign data wrapper Column names were not quoted leading to the possibility of invalid SQL output. The existing code allowed reserved words to be used as column names, this creates problems when trying to run the output in postgresql. No exit code to let the calling program know if there has been an OGR Error

In addition I have introduced MAX_IDENTIFIER_LEN to be clearer about creating identifiers that are no longer than the maximum length allowed in postgresql

I would be happy to make any amendments if you are happy to merge with changes.

Many thanks

Kieran

pramsey commented 4 years ago

One more request (I'm starting to feel like That Guy) ... Could you add a tiny xslx file to the data directory and a set of tests that exercise it, accessing sheets by name and by index number? Basically some tests to exercise your new functionality. In the pgsql test set you could also maybe add some tests to exercise your keyword quoting magic?

mccuskk commented 4 years ago

Hi

Sorry for the long pause (family problems). Where do you want the tests and how would you prefer them to be scripted? I couldn't find any examples in this project to copy. Sorry it's not something I have any experience in.

pramsey commented 4 years ago

The test file would go in ./data/ the script would go in ./input/ and the expected output goes in ./output/. The whole thing is run by the make installcheck target. It's a bit fiddly, in that it first runs a substitution on the "input" script, which creates a "sql" script, and also on the "output" script, which creates an "expected" file, and then the outputs from the "sql" file are compared with the "expected" file. So it takes a little while sometimes to figure out what is landing where. The file.source test isn't a bad one to learn from given your test data.

mccuskk commented 4 years ago

okay I've had a look at the testing and figured it out. The issue I have now is that the changes I have made affect the ogr_fdw_info tool which doesn't appear to have any tests at the moment e.g. the accessing of sheets by index number is only relevant to ogr_fdw_info and likewise the magic quoting of reserved words.

Let me know what you want and I'll happily do it. Thanks Kieran