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

Add diff-with-file option to copy command #729

Merged
merged 20 commits into from
Jul 5, 2023

Conversation

upsicleclown
Copy link
Collaborator

@upsicleclown upsicleclown commented Jun 26, 2023

  • Add ability to generate diff file by specifying --diff-with-file to the copy tool

@upsicleclown upsicleclown changed the title Update sqlx to 0.7.0-alpha.3 Add diff-with-file option to copy command Jun 26, 2023
@upsicleclown upsicleclown marked this pull request as ready for review June 27, 2023 20:11
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

looks good, thanks! we do need some more testing though - a test that shows that all corner cases are handled as expected:

  • same tile data
  • a tile is added
  • a tile is deleted

We may, as a separate PR, also want the mbtiles apply-diff src_file.mbtiles diff.mbtiles dst_file.mbtiles - but no rush on this

justfile Outdated Show resolved Hide resolved
martin-mbtiles/src/mbtiles_queries.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/mbtiles_queries.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
@nyurik
Copy link
Member

nyurik commented Jun 28, 2023

P.S. There is a merge conflict - you may want to simply delete and re-create the lock file after you fix the cargo.toml file merge.

@upsicleclown
Copy link
Collaborator Author

looks good, thanks! we do need some more testing though - a test that shows that all corner cases are handled as expected:

  • same tile data
  • a tile is added
  • a tile is deleted

@nyurik all of these test cases are addressed by diffing geography-class-jpg.mbtiles against the new file I created called geography-class-jpg-modified.mbtiles.

geography-class-jpg-modified.mbtiles has 1 row added, 1 row removed and 1 row modified. This is why I assert that the diff has 3 tiles rows. I do realize this assert_eq!(...,3) isn't very clear, but I'm not sure how to make it more transparent while also not making it needlessly complex.

@upsicleclown upsicleclown force-pushed the copy-diff-mbtiles branch 3 times, most recently from d2efcab to 1f22f86 Compare June 29, 2023 18:47
@upsicleclown upsicleclown requested a review from nyurik June 29, 2023 18:48
@upsicleclown
Copy link
Collaborator Author

The failing test is caused by the fact that the CI pipeline doesn't install sqlite. Sqlite is needed with these changes to be able to run the corresponding integration tests in test.sh. However, I have an upcoming change that makes it so that installing sqlite is not needed anyways: upsicleclown#2.
So I would prefer merging those changes into this branch and then merging this branch, so that I do not have to make a fix and then inevitably undo it.

@nyurik
Copy link
Member

nyurik commented Jun 30, 2023

agree, lets comment out the apply test in the test.sh for now. Eventually i would still like to have it as part of the test suite - this way our documentation always stays correct (i.e. we can always guarantee that we can apply the delta directly with sqlite3)

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

looks good, one minor nit (in addition to commenting out the call to sqlite3). BTW, you may want to add a "TODO: " comment - to only re-enable it once sqlite3 is installed on windows CI (and possibly mac?)

tests/fixtures/apply_diff.sh Outdated Show resolved Hide resolved
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

p.s. feel free to merge it once it passes the ci

@nyurik nyurik mentioned this pull request Jul 4, 2023
nyurik added a commit that referenced this pull request Jul 4, 2023
This is mostly a noop, just updating to the new way of storing cached
queries and a few other changes per
https://github.com/launchbadge/sqlx/blob/main/CHANGELOG.md#070---2023-06-30

Note that this uses some of the code from #729 (thanks @upsicleclown !)

A minor unrelated change - a reformat of `martin/release.toml`
@upsicleclown upsicleclown enabled auto-merge (squash) July 5, 2023 15:57
@upsicleclown upsicleclown merged commit e004908 into maplibre:main Jul 5, 2023
16 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.

None yet

2 participants