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

test: Fix substitution queries #56

Conversation

stepansergeevitch
Copy link
Collaborator

Fixed query substitution test

@stepansergeevitch stepansergeevitch requested a review from a team as a code owner July 31, 2024 11:29
@ptiurin
Copy link
Collaborator

ptiurin commented Aug 8, 2024

@stepansergeevitch can you please provide an example here of the changed query so I can better understand the change?

@stepansergeevitch
Copy link
Collaborator Author

@stepansergeevitch can you please provide an example here of the changed query so I can better understand the change?

It's a bit complicated. Basically, every string you're passing to a "query constructor" get's converted to a query parameter and query becomes parameterized. And as I described in a different PR, substitution logic in metabase is weird, and sometimes it passes parameters to a driver and sometimes it does the substitution itself. And in the latter we get weird issues. So the goal here is to reduce the amount to parameters that are generated during query construction.

Copy link
Collaborator

@ptiurin ptiurin left a comment

Choose a reason for hiding this comment

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

Fair enough, I think the logic here makes sense. At least there's no danger as far as I can tell.

@stepansergeevitch stepansergeevitch merged commit 44c1156 into develop Aug 12, 2024
2 checks passed
@stepansergeevitch stepansergeevitch deleted the FIR-35108-the-number-of-parameters-passed-does-not-equal-the-number-of-parameter-markers branch August 12, 2024 11:19
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