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

Percent in string literal incorrectly double encoded #297

Open
coltnz opened this issue Jan 28, 2024 · 5 comments
Open

Percent in string literal incorrectly double encoded #297

coltnz opened this issue Jan 28, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@coltnz
Copy link

coltnz commented Jan 28, 2024

Describe the bug

Percent in string literal incorrectly double encoded with sqlachemy interface.

Steps to reproduce

  1. Make a sql call via sqlalchemy query with a 'Format' in a literal string
    e.g.
    SELECT formatDateTime(toDate('2010-01-04'), '%g')

Result:


┌─formatDateTime(toDate('2010-01-04'), '%%g')─┐
│ %g                                          │
└─────────────────────────────────────────────┘

Expected behaviour

┌─formatDateTime(toDate('2010-01-04'), '%g')─┐
│ 10                                         │
└────────────────────────────────────────────┘

Configuration

On a slightly older version, don't see the necessary fix on master.

Analysis

Various places in code has:

        if self.preparer._double_percents:
            text = text.replace("%", "%%")
        return text

class ChIdentifierPreparer(IdentifierPreparer)

doesn't override the assignment in its base class:

        self._double_percents = self.dialect.paramstyle in (
            "format",
            "pyformat",
        )

Workaround

I monkey patched:

original_init = ChIdentifierPreparer.__init__

def new_init(self, *args, **kwargs):
    original_init(self, *args, **kwargs)
    self._double_percents = False

ChIdentifierPreparer.__init__ = new_init
@coltnz coltnz added the bug Something isn't working label Jan 28, 2024
@genzgd
Copy link
Collaborator

genzgd commented Jan 28, 2024

Thanks for the analysis and including a workaround!

I think you're the first person to open an issue about SQLAlchemy that's not related to Superset. I'd honestly recommend against trying to use clickhouse-connect SQLAlchemy for anything but Superset, since support is incomplete and tests are minimal. Nevertheless it's possible this will get fixed when we take a look at SQLAlchemy 2.0 support.

@coltnz
Copy link
Author

coltnz commented Jan 29, 2024

Streamlit uses sqlachemy for its caching so don't think switching is an easy option unfortunately.

@genzgd
Copy link
Collaborator

genzgd commented Jan 29, 2024

Thanks for the context -- I don't see sqlalchemy as a requirement for the latest streamlit install, do you happen to know if it's still required or what else I'm missing? That would help us decide the priority in fixing this.

@coltnz
Copy link
Author

coltnz commented Jan 31, 2024

From https://docs.streamlit.io/library/advanced-features/connecting-to-data

Step 1: Install prerequisite library - SQLAlchemy
All SQLConnections in Streamlit use SQLAlchemy.

@genzgd
Copy link
Collaborator

genzgd commented Jan 31, 2024

Thanks for the response! We'll take a look at Streamlit usage when determining the priorities for SQLAlchemy improvements and bug fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants