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

fix: Schema is not retaining the correct column order #73

Merged
merged 9 commits into from
Jul 22, 2024

Conversation

mjsqu
Copy link
Collaborator

@mjsqu mjsqu commented Jul 17, 2024

Fixes #62 by refactoring the desired_columns function to use lists instead of sets as they preserve order. List comprehensions are used to filter the lists rather than a loop. Operations such as intersection, difference and union are also replaced with list comprehensions.

Also includes modifications to the existing TestSelectsAppropriateColumns:

  • Adds more columns to strengthen proof that ordering is preserved
  • Assertion changed to compare two lists, will fail if they are not in the same order
  • Added a duplicate column to the select list

Adds new test TestInvalidInclusion which tests that an exception is raised when an inclusion value of "invalid" is supplied, which is not one of the valid values:

  • automatic
  • available
  • unsupported

@mjsqu mjsqu requested a review from s7clarke10 July 17, 2024 02:20
@mjsqu mjsqu self-assigned this Jul 17, 2024
@mjsqu mjsqu added the bug Something isn't working label Jul 17, 2024
@mjsqu
Copy link
Collaborator Author

mjsqu commented Jul 17, 2024

For my own future reference, I could have more quickly run my updated tests by doing:

poetry install
poetry run pytest tests/test_tap_mssql.py -k TestSelects -v
poetry run pytest tests/test_tap_mssql.py -k TestInvalid -v

Running all tests requires the build of a database (as supplied by the docker image in the tox.ini) - but the functionality I was testing didn't require a connection to SQL Server.

Copy link
Collaborator

@s7clarke10 s7clarke10 left a comment

Choose a reason for hiding this comment

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

Hi @mjsqu ,

I have reviewed the changes and like what you have done - thank you 🥇 .

My testing included.

  1. Peer Review of the code
  2. Compare the discovery of the schema (current vs PR)
  3. Compare the column ordering of loaded data in target-snowflake (current vs PR)
  4. Compare the content diff of result in target-snowflake (current vs PR).

All passed successfully.

Could we add the following changes however.

  1. Update the .bumpversion.cfg with new version
  2. Update the pyproject.toml with new version
  3. Update the CHANGELOG.md with description of latest version / change.
  4. Rebuild the pyproject lockfile 👍
   poetry lock --no-update

Thank you.

@mjsqu
Copy link
Collaborator Author

mjsqu commented Jul 22, 2024

These are done:

  • Update the .bumpversion.cfg with new version
  • Update the pyproject.toml with new version
  • Update the CHANGELOG.md with description of latest version / change.

I ran poetry lock --no-update but there are no changes as I have made no updates to dependencies

@s7clarke10
Copy link
Collaborator

Thank you for adding the version updates. Change approved. Thank you.

@mjsqu mjsqu merged commit e4f90bd into wintersrd:master Jul 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema is not retaining the correct column order
2 participants