Fix params substitution for select queries

[dmkulazhenko]

Select queries use substitution of arguments using % formatting, which doesn't work as far as args passed as Mapping.
This PR changes substitution formatting mechanism to str.format method.

[dmkulazhenko]

oh, probably a duplicate of #138... in any case I really need this logic 😢
CC @misuzu, @stankudrow

[stankudrow]

@dmkulazhenko , hello.

LGTM, tests passed, mypy is OK with these files.

@misuzu

[dmkulazhenko]

@stankudrow, changelog entry and test for KeyError trigger added

[ayratsa]

@misuzu, please, proceed with the PR

[stankudrow]

@dmkulazhenko , hello.

Any updates about the suggestions above?

[stankudrow]

@dmkulazhenko , hello.

Are you willing to finish this PR including the suggestions above?

[dmkulazhenko]

@dmkulazhenko , hello.

Are you willing to finish this PR including the suggestions above?

@stankudrow, maybe I get it wrong, but what should I do?

• changelog update
• test added
• question regarding try...except answered

---

upd: seems to be you expect that I will drop try...except, I don't mind so much about it, so will do it in a couple of minutes

[dmkulazhenko]

@stankudrow, re-review pls

[stankudrow]

@stankudrow, re-review pls

@dmkulazhenko , done.

@DaniilAnichin , could you also have a look, please? Looks good to me, that last time I ran mypy over these files the checker wasn't unpleased.

[DaniilAnichin]

Looks good, no comments

[baconfield]

This release might want a warning/heads up for those upgrading, as this breaks existing code that already uses the %(name)s formatting in queries

[stankudrow]

This release might want a warning/heads up for those upgrading, as this breaks existing code that already uses the %(name)s formatting in queries

Hello.

Could you possibly list here a small example that demonstrates your point?

Also, please do feel free to open a PR (if you like) that 1) does not decline the changes (to a certain extent) from this PR and 2) allows to handle the case you have mentioned before so the compatibility wouldn't break. It is possible to mitigate risks while this work isn't really released.

[baconfield]

Could you possibly list here a small example that demonstrates your point?

The simplest example I have would be a query like this:

await self.cursor.execute(
    """
    SELECT
        EXISTS(
            SELECT 1
            FROM table_a
            WHERE profile_id = %(profile_id)s
        ) AS a_enabled,
        EXISTS(
            SELECT 1
            FROM table_b
            WHERE profile_id = %(profile_id)s
        ) AS b_enabled
    """,
    {"profile_id": profile_id}
)

Also, please do feel free to open a PR (if you like) that 1) does not decline the changes (to a certain extent) from this PR and 2) allows to handle the case you have mentioned before so the compatibility wouldn't break. It is possible to mitigate risks while this work isn't really released.

Another parameter for setting this came to mind, but imagine that would be pretty clunky. The other alternative was this, but unsure of this possibly having gotchas down the line.

    @staticmethod
    def substitute_params(query: str, params: Mapping[str, Any]) -> str:
        if not isinstance(params, Mapping):
            raise ValueError("Parameters are expected to be a mapping")

        first_param = next(iter(params))
        if f"{{{first_param}}}" in query:
            return query.format(**escape_params(params))
        else:
            return query % escape_params(params)

[stankudrow]

Could you possibly list here a small example that demonstrates your point?

The simplest example I have would be a query like this:

await self.cursor.execute(
    """
    SELECT
        EXISTS(
            SELECT 1
            FROM table_a
            WHERE profile_id = %(profile_id)s
        ) AS a_enabled,
        EXISTS(
            SELECT 1
            FROM table_b
            WHERE profile_id = %(profile_id)s
        ) AS b_enabled
    """,
    {"profile_id": profile_id}
)

Also, please do feel free to open a PR (if you like) that 1) does not decline the changes (to a certain extent) from this PR and 2) allows to handle the case you have mentioned before so the compatibility wouldn't break. It is possible to mitigate risks while this work isn't really released.

Another parameter for setting this came to mind, but imagine that would be pretty clunky. The other alternative was this, but unsure of this possibly having gotchas down the line.

    @staticmethod
    def substitute_params(query: str, params: Mapping[str, Any]) -> str:
        if not isinstance(params, Mapping):
            raise ValueError("Parameters are expected to be a mapping")

        first_param = next(iter(params))
        if f"{{{first_param}}}" in query:
            return query.format(**escape_params(params))
        else:
            return query % escape_params(params)

Hm, what to try, e.g., this solution?

And the snippet may be like this:

@baconfield , would you like to open a PR and try this or any better solution or should I do this?

Also, consider updating tests on:

• C-style strings;
• formatable strings;
• test no, one and multiple (two will be enough) placeholders for both styles.

[baconfield]

would you like to open a PR and try this or any better solution or should I do this?

Yeah that looks like a better approach, I don't mind making a PR, it will just be a bit as I'll need to wait until after hours tonight.

[dmkulazhenko]

@stankudrow @baconfield , tbh it's totally my fault... I feel really stupid right now, because I had no idea that old python format style have an ability to pass arguments ("%(arg)s" % {"arg": "val"} == "val").

So, the whole idea of this PR is pointless... My initial problem was just lack of knowledge of old-style python formatting 😢. I believe @barakor had same intentions in #138.

There is 3 options to do now:

1. revert this PR (looks like the easiest and most valid option) and do a release;
2. make some generic approach that will determine parameters style on-the-fly (looks cumbersome and kinda pointless);
3. force users to write queries in new style (bad behavior in global + adds a lot of headache to libs where asynch used as upstream e.g. https://github.com/nebuly-ai/clickhouse-sqlalchemy).

If with 1st and 2nd options implementation is clear, 3rd option will require changes not only in clickhouse-sqlalchemy, but also in sqlalchemy:

• https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/sql/compiler.py#L267
• https://github.com/sqlalchemy/sqlalchemy/blob/d96c04950d6a1a72961a097289f7fd850319e3d0/lib/sqlalchemy/engine/interfaces.py#L268

As you can see there is no option like "{name}" 😞.

---

To mitigate such PRs in future it's a good idea to add section with an example of parametrized query or a link to https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting. Btw, finding this documentations by googling something like "python named percent formatting" is not that easy, thanks to dummy guides copy-pasters 🙃.