Skip to content

feat: add support for table column mapping #45

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

Merged
merged 3 commits into from
Feb 10, 2025
Merged

Conversation

dlclark
Copy link
Contributor

@dlclark dlclark commented Feb 8, 2025

This PR adds a feature to map from the model field names to specific database column names.

My use case is that I need to write queries that include table aliases in the where clauses (e.g. select a.id, a.someval, b.id, b.someval from table1 a inner join table2 b on a.id=b.otherid where a.id=?) but I'm unable to represent a.id in the where clause generated by mql today.

To fix this I've added a new mapping option for overriding the model's field name in the where clause. The default behavior is left untouched, but any field name found in the withTableColumnMap (an exact match) will be swapped out for the mapped value when generating the where clause string.

@dlclark dlclark requested review from a team as code owners February 8, 2025 07:12
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Just a few general items to do:

  • Missing change log entry.
  • Need to re-generate coverage (make converage) and commit changes

@jimlambrt
Copy link
Collaborator

@dlclark -> Ty for the contribution. It looks great. just a few minor bits here and there. Also, the workflows need to be updated which I've opened a PR for against main (#46). Once that's approved and merged, it would be great if you could rebase on that so the workflows can run successfully on your PR. Thanks again.

@dlclark
Copy link
Contributor Author

dlclark commented Feb 9, 2025

@jimlambrt ok, in theory I've addressed the comments here, let me know if anything else jumps out.

I'll re-base once the other PR is done.

@dlclark
Copy link
Contributor Author

dlclark commented Feb 9, 2025

@jimlambrt re-base complete.

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Ty for the updated commits. I think, we're close. I think you need to run make coverage again and check in the updated coverage.log

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Ty again for the contribution.

@jimlambrt jimlambrt merged commit d83d299 into hashicorp:main Feb 10, 2025
11 checks passed
@jimlambrt
Copy link
Collaborator

@dlclark do you need a release, or are you happy to just pull the latest into your project?

@dlclark
Copy link
Contributor Author

dlclark commented Feb 10, 2025

I'm good either way

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.

2 participants