Skip to content

Conversation

@zilto
Copy link
Collaborator

@zilto zilto commented Sep 29, 2025

Solves issue #3139

Problem

When creating the ATTACH statement for the ducklake client, password value from SQLAlchemy would render as *** instead of its value. This was not caught in our CI / tests because we use sqlalchemy 1.4 and this behavior seems to be only in sqlalchemy 2.0

Changes

  • change db_url = catalog.to_url() -> db_url = catalog.to_url().render_as_string(hide_password=False)
  • misc:
    • replaced posixpath with pathlib (posixpath is an internal module as its docstring mentions)
    • remove unused imports

@zilto zilto self-assigned this Sep 29, 2025
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 29, 2025

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
docs 8399bf4 Commit Preview URL

Branch Preview URL
Sep 30 2025, 02:01 PM

@zilto zilto added the bug Something isn't working label Sep 29, 2025
adds the ducklake `Catalog` to your duckdb database
"""
from __future__ import annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

A question for understanding - this is the first time I'm seeing this and looked it up -> Is the reason to use this instead of a double quoted forward reference the fact that, starting from python 3.11, all annotations are stored as strings by default and we can just remove this when python 3.10 is no longer supported (instead of going over double quoted references)? 👀

Copy link
Collaborator Author

@zilto zilto Sep 30, 2025

Choose a reason for hiding this comment

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

In short:

  • When you import or execute a Python file, it is read from top to bottom.
  • This created issues for the earliest Python versions of type annotations.
    class A:
       def __init__(self): ...
       
       # can't annotate `B` because it is not defined yet when going top-to-bottom
       def get_b(self) -> B: ...
       
    class B:
      ...   
  • The first solution was to define types as strings
    # keeping everything else the same
    class A:
      # ...
      def get_b(self) -> "B": ...
  • This is error-prone and didn't support IDE features (e.g., can't rename class, navigate to class definition)
  • Python added the built-in from __future__ import annotations. This means the full file is read from top-to-bottom and all the import mechanism is resolved before annotations are added to Python objects
  • This allows to write the previously problematic def get_b(self) -> B: ... even if B isn't defined yet because B will be defined by the time the file and imports are resolved.

TL;DR, we can and we should have from __future__ import annotations on all files. It's common to have this as a linting rule. It hasn't enforced as the default behavior either

from future import annotations was previously scheduled to become mandatory in Python 3.10, but the Python Steering Council twice decided to delay the change (announcement for Python 3.10; announcement for Python 3.11). No final decision has been made yet.

anuunchin
anuunchin previously approved these changes Sep 30, 2025
Copy link
Contributor

@anuunchin anuunchin left a comment

Choose a reason for hiding this comment

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

🧠

@zilto zilto force-pushed the fix/3139-ducklake-sqlalchemy-credentials branch from edb6a13 to 8399bf4 Compare September 30, 2025 13:55
@zilto zilto merged commit 4acdca7 into devel Sep 30, 2025
68 checks passed
@zilto zilto deleted the fix/3139-ducklake-sqlalchemy-credentials branch September 30, 2025 15:58
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

Successfully merging this pull request may close these issues.

3 participants