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

[ch98965] Error importing geopackage files with multiple layers #15907

Merged

Conversation

Shylpx
Copy link
Collaborator

@Shylpx Shylpx commented Oct 30, 2020

Resources

Context

  • When a geopackage file has multiple layers, in order to create a dataset for each layer (max 50), the CartoDB::Importer2::GpkgSplitter checks the number of layers. To get this, it uses the ogrinfo command that gives information about the layers (name, type), and it is parsed using regular expressions to extract the name of each layer.
  • The error was in that regular expressions, because the information of each layer is something like 1: layer_name (Line String), and the splitter removes the number before the layer name and the geometry between parenthesis before the name. But for a specific type of layer geometry, it fails: 2: layer_name (3D Measured Unknown (any)). Because the regular expression was not considering that the string between parenthesis could have also parenthesis to be removed.
  • The result is a geopackage with 16 layers with that geometry, and the splitter understand that there are 0 layers, so the result is trying to import all the layers with the same name and of course it fails because the layer already exists when tries to import the second one.

Changes

  • Fix regular expression to extract the layer name correctly.
  • Update test to consider the corner case.
  • Minor Rubocop fixes.

@Shylpx Shylpx self-assigned this Oct 30, 2020
@Shylpx Shylpx marked this pull request as ready for review November 5, 2020 12:59
@Shylpx Shylpx requested a review from rafatower November 5, 2020 13:42
Copy link
Contributor

@rafatower rafatower left a comment

Choose a reason for hiding this comment

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

👍 thanks for the fix.

I re-triggered the builds cancelled:

HEAD is now at 30fce9d Merge branch 'master' into feature/ch98965/initiative-error-uploading-a-geopackage-file
ERROR
ERROR: error fetching git source: context canceled

because we should make sure there's no regression test broken (unlikely, but better safe - e.g: there is another gpkg file with tests associated to it). Please, don't cancel CI builds unless there's a good reason to do so 😉

Thanks! :)

@Shylpx Shylpx merged commit ecbfee4 into master Nov 10, 2020
@thedae thedae mentioned this pull request Nov 20, 2020
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.

None yet

2 participants