sqlalchemy / sqlalchemy2-stubs

PEP-484 typing stubs for SQLAlchemy 1.4
MIT License
159 stars 41 forks source link

Static typing using Cast expects two type arguments #115

Open mdczaplicki opened 3 years ago

mdczaplicki commented 3 years ago

Describe the bug We have a statically typed dictionary that has values of type Cast[Decimal] & it seems to be valid, since we cast column value to FLOAT. But the type annotation expects two type arguments. I think it uses this stub: https://github.com/sqlalchemy/sqlalchemy2-stubs/blob/master/sqlalchemy-stubs/sql/elements.pyi#L333 whereas it's for a method cast on ColumnElement.

Expected behavior Static typing using Cast[Decimal] works fine :P

To Reproduce

from uuid import uuid4, UUID
from typing import Dict
from sqlalchemy.sql.elements import Cast
from decimal import Decimal
from sqlalchemy import FLOAT, cast

class X:
  y: Optional[float]
x =x()
x.y = 2.0

payload: Dict[UUID, Cast[Decimal]] = {
    uuid4(): cast(x.y, FLOAT)
}

Error

error: "Cast" expects 2 type arguments, but 1 given

Versions.

Additional context Maybe we should actually not use cast in this case? We are using it because y is an Optional[float] & not float, but we are 100% sure that it's not None?

Have a nice day!

mdczaplicki commented 3 years ago

@MaicoTimmerman, bump :stuck_out_tongue:

zzzeek commented 3 years ago

SQLAlchemy's Cast accepts a TypeEngine object, not a Python type. So here the more correct thing would be cast[sqlalchemy.types.Numeric]. We don't express SQL constructs in terms of Python datatypes, there is a layer of indirection from the TypeEngine.

mdczaplicki commented 3 years ago

But this still would be one type argument, not two; isn't that right?

zzzeek commented 3 years ago

from my POV it seems like you care about the resulting type of the construct, not the originating type, so yes. but i may not be seeing the whole picture.

mdczaplicki commented 3 years ago

Yeah, that makes sense that I've approached from the different direction. But nevertheless the issue still stands as present. Unless I need to provide both originating & resulting types. I'll check tomorrow.

MaicoTimmerman commented 3 years ago

I've looked into the originating+resulting vs resulting only generic type. I believe to be fully typed, we need both. Cast exposes both the clause and wrapped_column_expression properties, which should be typed based on the source type, requiring 2 generic arguments.

However we'd need to do some more thinking about the source type to be ORM compatible. Currently, it expects a source to be inherited from ClauseElement, however ORM mapped instance attributes are python types:

from sqlalchemy import cast, Float, Column, Numeric
from sqlalchemy.orm import declarative_base

Base = declarative_base()

class X(Base):
    y = Column(Float)

x = X()
x.y = 2.0
reveal_type(x.y)
reveal_type(cast(x.y, Numeric))  # error: Value of type variable "_CLE" of "Cast" cannot be "float"
reveal_type(cast(X.y, Numeric))  # works

I also noted that the generic is currently Cast[<result_type>, <source_type>], which I'd find to be counter-intuitive, as the generic order is reversed from the calling argument order.

zzzeek commented 3 years ago

I've looked into the originating+resulting vs resulting only generic type. I believe to be fully typed, we need both. Cast exposes both the clause and wrapped_column_expression properties, which should be typed based on the source type, requiring 2 generic arguments.

Well the ".clause" of the Cast is really not important on the outside, it's there for informational purposes but does not determine any meaningful behavior of the construct, only the specifics of the string that is generated in compilation. There has to be some kind of boundary for what we put in the [] of a generic type. If I have an object that has ".x = int, .y = dict, .foo = Bar()" as members, we hardly would normally be passing around things like MyObject[int, dict, Bar] - there's some threshold of what's actually important to be in the generic slot, and in this case 100% of the behavior of the object as manipulated in Python and used in idiomatic patterns is determined by the target type, not the source type.

I also noted that the generic is currently Cast[<result_type>, <source_type>], which I'd find to be counter-intuitive, as the generic order is reversed from the calling argument order.

I don't think every member of an object that happens to have a SQL type needs to be part of the generic representation, and perhaps we should try to clarify what really goes into the [] and why.

anentropic commented 2 years ago

based on this issue I had some code like:

    rows = await session.execute(
        sql.select([func.pg_try_advisory_xact_lock(cast(key, BigInteger))])
    )

where key is a python integer, i.e. just a literal value for SQL and not a field expression

this worked fine in my unit tests, but mypy complains:

error: Value of type variable "_CLE" of "Cast" cannot be "int"

After a bit of experimenting I found this formulation that works and also satisfies mypy:

    rows = await session.execute(
        sql.select([func.pg_try_advisory_xact_lock(literal(key, BigInteger))])
    )
peterschutt commented 2 years ago

cast() accepts the type as the first argument and the value as second arg, eg, cast(BigInteger, key).

anentropic commented 2 years ago

cast() accepts the type as the first argument and the value as second arg, eg, cast(BigInteger, key).

I think that's mypy cast ... here I am talking about from sqlachemy import cast

peterschutt commented 2 years ago

Oh my bad! Totally read it as typing cast