Fixed #17260 -- Added time zone aware aggregation and lookups. #715

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
4 participants
@aaugustin
Member

aaugustin commented Feb 11, 2013

No description provided.

@akaariai

View changes

django/contrib/gis/db/models/sql/compiler.py
@@ -260,6 +262,7 @@ class SQLUpdateCompiler(compiler.SQLUpdateCompiler, GeoSQLCompiler):
class SQLAggregateCompiler(compiler.SQLAggregateCompiler, GeoSQLCompiler):
pass
+# TODO
class SQLDateCompiler(compiler.SQLDateCompiler, GeoSQLCompiler):

This comment has been minimized.

@akaariai

akaariai Feb 11, 2013

Member

TODO: Remove this TODO.

@akaariai

akaariai Feb 11, 2013

Member

TODO: Remove this TODO.

+ offset = len(self.query.extra_select)
+ for rows in self.execute_sql(MULTI):
+ for row in rows:
+ datetime = row[offset]

This comment has been minimized.

@akaariai

akaariai Feb 11, 2013

Member

maybe move this inside the elif branch?

@akaariai

akaariai Feb 11, 2013

Member

maybe move this inside the elif branch?

This comment has been minimized.

@aaugustin

aaugustin Feb 11, 2013

Member

It's useful if neither the if nor the elif branch are taken. If I moved it in elif I'd have to repeat it in a final else clause.

Besides, this code predates my patch.

@aaugustin

aaugustin Feb 11, 2013

Member

It's useful if neither the if nor the elif branch are taken. If I moved it in elif I'd have to repeat it in a final else clause.

Besides, this code predates my patch.

This comment has been minimized.

@akaariai

akaariai Feb 11, 2013

Member

Sorry for the noise, somehow read that as if-else.

@akaariai

akaariai Feb 11, 2013

Member

Sorry for the noise, somehow read that as if-else.

@@ -397,6 +399,9 @@ class BaseDatabaseFeatures(object):
# Can datetimes with timezones be used?
supports_timezones = True
+ # Does the database have a copy of the zoneinfo database?
+ has_zoneinfo_database = True

This comment has been minimized.

@akaariai

akaariai Feb 11, 2013

Member

It might be a good idea to add a convert_tz_to_dbtz method to connection.ops. I expect some databases will have timezones but with different names than used by Django. For these DBs it would be useful to be able to convert at least the most common timezones from django to db format.

EDIT: this isn't actually that much related to the has_zoneinfo_database feature...

@akaariai

akaariai Feb 11, 2013

Member

It might be a good idea to add a convert_tz_to_dbtz method to connection.ops. I expect some databases will have timezones but with different names than used by Django. For these DBs it would be useful to be able to convert at least the most common timezones from django to db format.

EDIT: this isn't actually that much related to the has_zoneinfo_database feature...

This comment has been minimized.

@aaugustin

aaugustin Feb 11, 2013

Member

As far as I can tell, everyone uses the zoneinfo database.

If there's some demand for a mapping mechanism, it can be added it later on in a backwards compatible fashion.

Per the YAGNI principle, I haven't written it, and my current answer is "implement a tzinfo subclass whose tzname() method returns the appropriate value".

@aaugustin

aaugustin Feb 11, 2013

Member

As far as I can tell, everyone uses the zoneinfo database.

If there's some demand for a mapping mechanism, it can be added it later on in a backwards compatible fashion.

Per the YAGNI principle, I haven't written it, and my current answer is "implement a tzinfo subclass whose tzname() method returns the appropriate value".

This comment has been minimized.

@akaariai

akaariai Feb 11, 2013

Member

Yeah, later on if there is demand seems good enough.

@akaariai

akaariai Feb 11, 2013

Member

Yeah, later on if there is demand seems good enough.

+ # don't support time zone. Restore the zone used in the query.
+ if settings.USE_TZ:
+ datetime = datetime.replace(tzinfo=None)
+ datetime = timezone.make_aware(datetime, self.query.tzinfo)

This comment has been minimized.

@akaariai

akaariai Feb 11, 2013

Member

I don't get this comment + code. So, how is no tz support in the DB related to the code here?

@akaariai

akaariai Feb 11, 2013

Member

I don't get this comment + code. So, how is no tz support in the DB related to the code here?

This comment has been minimized.

@aaugustin

aaugustin Feb 11, 2013

Member

It's complicated :) Let's consider what happens when a query is executed with datetime_trunc_sql under USE_TZ = True.

Only one of the supported databases has proper time zone support: PostgreSQL. In this situation, it returns naive datetimes, which are used by Django as-is. While this is a reasonably safe choice, it isn't appropriate for Django, because we promise that the ORM will always return aware datetimes. Restoring the time zone used by the query makes sense here.

The other databases also return naive datetimes, but Django bolts on the UTC timezone. This is clearly wrong here. I wipe it and restore the time zone used by the query. That's what the comment attempts to convey.

@aaugustin

aaugustin Feb 11, 2013

Member

It's complicated :) Let's consider what happens when a query is executed with datetime_trunc_sql under USE_TZ = True.

Only one of the supported databases has proper time zone support: PostgreSQL. In this situation, it returns naive datetimes, which are used by Django as-is. While this is a reasonably safe choice, it isn't appropriate for Django, because we promise that the ORM will always return aware datetimes. Restoring the time zone used by the query makes sense here.

The other databases also return naive datetimes, but Django bolts on the UTC timezone. This is clearly wrong here. I wipe it and restore the time zone used by the query. That's what the comment attempts to convey.

+ cursor = self.connection.cursor()
+ cursor.execute("SELECT 1 FROM mysql.time_zone LIMIT 1")
+ return cursor.fetchone() is not None
+

This comment has been minimized.

@alex

alex Feb 15, 2013

Member

Please close your cursor when you're done.

@alex

alex Feb 15, 2013

Member

Please close your cursor when you're done.

This comment has been minimized.

@carljm

carljm Feb 15, 2013

Member

FWIW, most places in the database backends that open a cursor for a check like this don't explicitly close it (e.g. the supports_transactions check). Definitely worth fixing, but would make just as much sense to do across the board in a separate commit.

@carljm

carljm Feb 15, 2013

Member

FWIW, most places in the database backends that open a cursor for a check like this don't explicitly close it (e.g. the supports_transactions check). Definitely worth fixing, but would make just as much sense to do across the board in a separate commit.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Feb 16, 2013

Member

Merged in e74e207.

Member

aaugustin commented Feb 16, 2013

Merged in e74e207.

@aaugustin aaugustin closed this Feb 16, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment