kayak / pypika

PyPika is a python SQL query builder that exposes the full richness of the SQL language using a syntax that reflects the resulting query. PyPika excels at all sorts of SQL queries but is especially useful for data analysis.
http://pypika.readthedocs.io/en/latest/
Apache License 2.0
2.55k stars 298 forks source link

Cannot build query with field named "field" #623

Open thunderk opened 3 years ago

thunderk commented 3 years ago

Maybe it comes from my way of using Query, but here is a simple example to demonstrate:

from pypika.queries import Schema, Query

table=Schema("schema").table

print(Query.from_(table).select(table.notfield))
# outputs: SELECT "notfield" FROM "schema"."table"

print(Query.from_(table).select(table.field))
# outputs: SELECT <bound method Selectable.field of Table('table', schema='<pypika.queries.Schema object at 0x7fa69f56ff60>')> FROM "schema"."table"
thunderk commented 3 years ago

Giving more thought about it, it's logical, I should use Field("field", table=table) instead of the attribute shortcut.

But I think it's confusing to allow getattr access, but that it's working only if there isn't a method/attribute/property of the same name in the Table class.

anhqle commented 3 years ago

Yes, it's a bit confusing, but that provides us the convenience of writing mytable.any_column_name_you_want without having to define that Field explicitly

thunderk commented 3 years ago

I understand the convenience it provides, but I still think it's a risky implementation that can break users' code in the future.

If a user has code that uses a mytable.data field, and then pypika adds a data attribute (or method, or property) to the Table implementation, the user code would break on upgrade, because the attribute access doesn't go through __getattr__ anymore.

I'd suggest to have something like mytable.fields.any_column_name_you_want, where fields would be an object with only the getattr mechanism, and no other attributes, keeping the convenience but without the name clash risk. The current __getattr__ can even redirect to the new system for backward compatibility (maybe with a depreciation warning).

What do you think?

Edit: backquoting

anhqle commented 3 years ago

Your point about breaking users' code makes sense. I'm not a core dev though, so I can't opine on whether the convenience trade-off is worth it :)