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 option to ignore columns in equality test #734

Closed
brunocostalopes opened this issue Nov 29, 2022 · 6 comments · Fixed by #765
Closed

Add option to ignore columns in equality test #734

brunocostalopes opened this issue Nov 29, 2022 · 6 comments · Fixed by #765
Assignees
Labels

Comments

@brunocostalopes
Copy link
Contributor

Describe the feature

I would like to be able to specify a list of columns to exclude on an equality test.

Describe alternatives you've considered

We can of course use compare_columns to list all columns, except the ones we want to exclude, but this leads to large yml files, difficult to maintain as our models evolve.

Additional context

Not database specific, it can be implemented across all different databases.

Who will this benefit?

This feature would be great for anyone who wants to compare data across environments where, for example, some of the fields are masked in one of the environments.
Another scenario where this can be useful is when updating the logic on a small subset of fields in a model and wanting to compare the development model with the production one, but obviously excluding the updated fields (which are expected to be different)

Are you interested in contributing this feature?

I have implemented this locally already. Happy to raise a PR if people think this feature is helpful.

@brunocostalopes brunocostalopes added enhancement New feature or request triage labels Nov 29, 2022
@deanna-minnick deanna-minnick self-assigned this Dec 6, 2022
@deanna-minnick
Copy link
Contributor

@joellabes What do you think about moving forward with this? I'd be game to put some work into this in the next couple of weeks!

@joellabes
Copy link
Contributor

@brunocostalopes thanks for this! I am very on board with this as an idea.

Some implementation notes:

  • The new argument should be ignore_columns (as opposed to exclude; let me know if you have a different opinion)
  • If both compare_columns and ignore_columns are provided, it should throw an error (see how union_relations does it)
  • Please also add some integration tests which exercise the new behaviour. I went looking for the existing integration tests for this test and didn't find any, so instead here's an example from the recency test. Note in particular that you can check that the test catches failing records by setting error_if to <1.

Since it's your issue and you've got the code already to hand, you get first dibs on implementing it, but if you're not going to have time to do the other things then it looks like @deanna-minnick is also willing to lend a hand - just let us know on the issue thread.

Let me know if you run into any issues 🎉

@brunocostalopes
Copy link
Contributor Author

@joellabes sounds good, thank you for the implementation notes. I'll take a first stab at it and if it seems like I'm stuck I'll ask @deanna-minnick for a bit of help.

@brunocostalopes brunocostalopes changed the title Add option to exclude columns in equality test Add option to ignore columns in equality test Dec 8, 2022
@brunocostalopes
Copy link
Contributor Author

brunocostalopes commented Dec 8, 2022

@joellabes I've raised the PR.

Please feel free to comment and suggest any changes, particularly if you have any suggestions on how to compare the list of columns in the ignore list with the list returned by the adapter (feels a bit hacky with the lowercase but couldn't think of a better approach so maybe I'm just missing something).

EDIT: just realised that I was getting the quoted value from the column not the name 🤦 . Still, happy to hear what others think of what would be the best way of making that comparison. I guess we can have this discussion in the PR.

@github-actions
Copy link

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 19, 2023
@github-actions
Copy link

Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest. Just add a comment to notify the maintainers.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants