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

Fixes for ms-sql import with GDAL #7521

Merged
merged 2 commits into from Aug 8, 2018

Conversation

stev-0
Copy link
Contributor

@stev-0 stev-0 commented Aug 2, 2018

Fix quotes on postgis import, add parameter for field selection
and fix mysql datsource formatting

Description

The OGR2OGR command was not working correctly for importing MS-SQL layers into Postgis via OGR. This also adds field selectors. Sorry the last PR on a similar issue was probably a bit ambitious for a first one - hopefully this one is a bit more self-contained.

Previous OGR2OGR command

ogr2ogr.exe -progress --config PG_USE_COPY YES -f PostgreSQL PG:" "host=pg_host port=pg_port dbname=pg_db password=pg_pwd active_schema=public user=pg_usr" " -lco DIM=2 "dbname='mssql_db' host=msql_host estimatedmetadata=true srid=27700 type=MultiPolygon table=\"dbo\".\"mssql_table\" (Shape) sql=" dbo.mssql_table -overwrite -lco GEOMETRY_NAME=geom -lco FID=id -nln public.new_tbl-nlt PROMOTE_TO_MULTI

New OGR2OGR command

ogr2ogr.exe -progress --config PG_USE_COPY YES -f PostgreSQL "PG:host=pg_host port=pg_port dbname=pg_db password=pg_pwd active_schema=public user=pg_usr" -lco DIM=2 "MSSQL:database=mssql_db;server=msql_host;trusted_connection=yes;tables=mssql_table" "dbo.mssql_table" -overwrite -lco GEOMETRY_NAME=geom -lco FID=id -nln public.new_tbl -nlt PROMOTE_TO_MULTI

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Nice!

Can I request that you split up the two changes into two different commits? The bug fix parts need to be backported to 3.2, but the addition of the fields choice is a new feature and cannot be backported.

@@ -346,7 +362,7 @@ def ogrConnectionStringAndFormatFromLayer(layer):
ogrstr += dsUri.schema() + "."

ogrstr += dsUri.table()
format = 'OCI'
format = 'OCI'
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove trailing space (actually this should be done by running the pre-commit script, as it will apply all formatting options required to get Travis to greenlight the change)

arguments.append(dimstring)
arguments.append(ogrLayer)
arguments.append(layername)
arguments.append('"{0}"'.format(ogrLayer))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change required? It should already be quoted if needed, and there's a chance this will cause double-quotes for some layer names

@stev-0
Copy link
Contributor Author

stev-0 commented Aug 6, 2018

FYI the final commit to get the formatting right (had trouble running prepare-commit.sh on my system) relates to the feature, not the bugfix, so does not need to be backported.

@nyalldawson
Copy link
Collaborator

Very nice! One last little change - can you update the commit message for the fields change, and add "[feature]" at the start of the message? There's hooks setup so that the documentation team are notified, and it will ensure that this feature is included in the release notes for 3.4.

@stev-0
Copy link
Contributor Author

stev-0 commented Aug 7, 2018

Should be done now - thanks for taking me through PR no. 1!

@nyalldawson
Copy link
Collaborator

@stev-0

Perfect! Bring on the next one 😉

@nyalldawson nyalldawson merged commit 56e605b into qgis:master Aug 8, 2018
@nyalldawson
Copy link
Collaborator

(i'll backport the fix btw)

@stev-0 stev-0 deleted the gdal-postgis-fields branch August 13, 2018 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants