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

v.db.join: handle existing columns properly #3765

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

ninsbl
Copy link
Member

@ninsbl ninsbl commented Jun 3, 2024

Currently, v.db.join fails if all columns to join already exist in the target table, because v.db.addcolumn is called with an empty list of columns to create.

This PR addresses the issue, removes Python2 related code along with some refactoring. It also adds a test-suite.

@ninsbl ninsbl added bug Something isn't working CI Continuous integration vector Related to vector data processing Python Related code is in Python database Related to database management backport to 8.3 labels Jun 3, 2024
@ninsbl ninsbl added this to the 8.4.0 milestone Jun 3, 2024
@ninsbl ninsbl requested a review from griembauer June 3, 2024 22:44
@github-actions github-actions bot added module tests Related to Test Suite labels Jun 3, 2024
@ninsbl ninsbl requested a review from tmszi June 4, 2024 11:41
@echoix
Copy link
Member

echoix commented Jun 4, 2024

For the rest, I don't really know what to look out for, and the diff (even only the first commit) is too hard to follow on a phone, I would need to look hard on a computer (but ain't available until the end of weekend). The variable renames, import alias rename, and other fixes mixed with the actual fix makes it harder to see what is equivalent or not.

@ninsbl
Copy link
Member Author

ninsbl commented Jun 4, 2024

Sorry. An issue got introduced earlier, with the latest update of the module I guess. If all columns to join were present in the target table, the module started failing, which was no problem earlier. The list of "columns_to_add" was empty in that case and v.db.addcolumn then fails because columns to add are missing.

@griembauer
Copy link
Contributor

Sorry. An issue got introduced earlier, with the latest update of the module I guess. If all columns to join were present in the target table, the module started failing, which was no problem earlier. The list of "columns_to_add" was empty in that case and v.db.addcolumn then fails because columns to add are missing.

Thanks a lot for catching this! The remaining refactoring also looks good to me. It ran successfully on some test tables.

Copy link
Member

@tmszi tmszi 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 to me. Tested and works as expected.

Small note:

Maybe you can update module doc with that short info if columns names are same in joined table, same columns names will be update in the source vector map table.

Alternatively, there could be a flag that would ensure the renaming of the same columns and joining them instead of updating existing columns.

@ninsbl
Copy link
Member Author

ninsbl commented Jun 5, 2024

Small note:

Maybe you can update module doc with that short info if columns names are same in joined table, same columns names will be update in the source vector map table.

Alternatively, there could be a flag that would ensure the renaming of the same columns and joining them instead of updating existing columns.

Good points @tmszi will open a new issue for that and other further improvements of v.db.join.

@ninsbl ninsbl merged commit fcb6199 into OSGeo:main Jun 5, 2024
26 checks passed
@ninsbl ninsbl deleted the v_db_join_bugfix branch June 5, 2024 07:02
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Jun 17, 2024
* handle existing columns properly

* refactoring and removal of Python2 compatible code

* add a basic testsuite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI Continuous integration database Related to database management module Python Related code is in Python tests Related to Test Suite vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants