Skip to content

Comments

Ensure param substitution works for c-style formatted queries#147

Open
baconfield wants to merge 5 commits intolong2ice:devfrom
baconfield:dev
Open

Ensure param substitution works for c-style formatted queries#147
baconfield wants to merge 5 commits intolong2ice:devfrom
baconfield:dev

Conversation

@baconfield
Copy link

Fixing param substitution for %(name)s formatting, e.g. cases like:

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

#141 (comment)

@dmkulazhenko
Copy link
Contributor

#141 (comment)

Copy link
Contributor

@dmkulazhenko dmkulazhenko left a comment

Choose a reason for hiding this comment

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

LGTM!

But Im not sure if in global this approach should be here 🤔

As I said in #141 (comment) approach of supporting both c-style format and modern format looks kinda pointless. Maybe full revert of #141 is enough.

@baconfield @stankudrow @long2ice what do you think?

@stankudrow
Copy link
Contributor

LGTM!

But Im not sure if in global this approach should be here 🤔

As I said in #141 (comment) approach of supporting both c-style format and modern format looks kinda pointless. Maybe full revert of #141 is enough.

@baconfield @stankudrow @long2ice what do you think?

Hello,

Could you provide the rationale for the #141 ? What were reasons why formatting via the .format() method is viable? I meam, if not only C-style is enough, then this approach may work, otherwise, this PR should revert the previous.

@dmkulazhenko
Copy link
Contributor

LGTM!
But Im not sure if in global this approach should be here 🤔
As I said in #141 (comment) approach of supporting both c-style format and modern format looks kinda pointless. Maybe full revert of #141 is enough.
@baconfield @stankudrow @long2ice what do you think?

Hello,

Could you provide the rationale for the #141 ? What were reasons why formatting via the .format() method is viable? I meam, if not only C-style is enough, then this approach may work, otherwise, this PR should revert the previous.

I described situation in linked comment #141 (comment)

@baconfield
Copy link
Author

To double check before I add any commits, is the project wanting to support just %(name)s (pyformat/extended c-style) style param substitution, or both {name} and %(name)s styles?

@stankudrow
Copy link
Contributor

Hi there.

@baconfield , let's remove the format-featured param substitution. The reasons are as follows:

  1. We are reverting the Fix params substitution for select queries #141
  2. The format feature should be discussed first in the "Discussions" or "Issues" and then, if the community agrees, to be PR'ed

So, we can just restore the broken functionality and the new is to be discussed first. @baconfield , @dmkulazhenko , @DaniilAnichin , if you are agree, let's get C-style formatting back and that will be all for now. Also, please add the record in the CHANGELOG.

Then, we can focus on: #145 , #148 , #150 and #153 .

Also, @barakor, are you willing to rebase and finalise your PR #138 ?

@nils-borrmann-tacto
Copy link
Contributor

I agree with rolling back that change. format-style substitution also clashes with clickhouses native server-side parameters (which I'm working on supporting).

@stankudrow
Copy link
Contributor

stankudrow commented Feb 12, 2026

@long2ice , hello.

If you like this PR, you can merge it, the CHANGELOG may be updated in one of next PRs, perhaps in the #138 or any other.

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.

4 participants