Skip to content

Conversion problem between interval and timedelta #150

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

Closed
lelit opened this issue Jun 6, 2017 · 7 comments
Closed

Conversion problem between interval and timedelta #150

lelit opened this issue Jun 6, 2017 · 7 comments
Assignees

Comments

@lelit
Copy link
Contributor

lelit commented Jun 6, 2017

I'm using asyncpg 0.11.0 and I hit a problem with a table containing an INTERVAL column.

Consider the following script:

import asyncio
import asyncpg


SETUP_SQL = """\
CREATE TABLE schedules (
  id serial not null primary key,
  name text,
  delay interval
)
"""

INSERT_SQL = """
INSERT INTO schedules (name, delay)
VALUES ('Foobar', '5 years')
"""

FETCH_SQL = """
SELECT delay
FROM schedules
WHERE name = 'Foobar'
"""

CLEANUP_SQL = """
DROP TABLE schedules
"""

async def run():
    conn = await asyncpg.connect(user='lele', password='lele',
                                 database='test', host='127.0.0.1')

    await conn.execute(SETUP_SQL)
    try:
        await conn.execute(INSERT_SQL)
        delay = await conn.fetchval(FETCH_SQL)
        print(delay)
    finally:
        await conn.execute(CLEANUP_SQL)

    await conn.close()


def main():
    loop = asyncio.get_event_loop()
    loop.run_until_complete(run())


if __name__ == '__main__':
    main()

It creates a sample table and inserts one record, with a delay value of five years, and executing it I get:

$ python test.py 
1800 days, 0:00:00

Commenting out the CLEANUP_SQL execution, with psql I get:

test=# select * from schedules;
 id |  name  |  delay  
----+--------+---------
  1 | Foobar | 5 years

and with pgcli, that uses psycopg2, I get:

lele@test> select * from schedules;
+------+--------+--------------------+
|   id | name   | delay              |
+======+========+====================+
|    1 | Foobar | 1825 days, 0:00:00 |
+------+--------+--------------------+

Why am I getting back 1800 days when using asyncpg?

@lelit
Copy link
Contributor Author

lelit commented Jun 9, 2017

Just for fun I tried to investigate a bit: effectively the interval_decode() function sees the result as “60 months, 0 days, 0 seconds and 0 microsecs”, and that explains the output (computed roughly as 30*months + days).
Then I lost some time trying to find references to how PG serializes its interval data type, with no luck.

OOC, where did you find the details underlying the protocol/codecs.pyx implementation?

@lelit
Copy link
Contributor Author

lelit commented Jun 9, 2017

I found the details in the PG sources (specifically, in backend/utils/adt/timestamp.c)...

I wonder if it'd make any sense in implementing a more PG-like interval Python data type, something like monthdelta but supporting also day fractions...

lelit added a commit to lelit/asyncpg that referenced this issue Jun 9, 2017
This is a POC, targeting issue MagicStack#150: PostgreSQL interval data type and Python timedelta have a
similar goal, but are subtly different because PG date arithmetic is much more versatile.

So, instead of trying to coerce an interval to a timedelta, the new class aims to carry the
same bits of information, and may implement a similar semantic, if needed.
@lelit
Copy link
Contributor Author

lelit commented Jun 9, 2017

This a quick experiment, please let me know if you think it could be acceptable. If so, I could implement remaining logic to allow a (minimal) interoperability with date and datetime.

@lelit
Copy link
Contributor Author

lelit commented Jun 11, 2017

Please give a look at my interval branch, this commit in particular: it implements most of the mentioned interoperability.

@lelit
Copy link
Contributor Author

lelit commented Jun 15, 2017

I'm looking forward a review of PR #150 🙏

@elprans
Copy link
Member

elprans commented Jun 19, 2017

I'm on vacation until next week. Will review once I'm back. Thanks!

@lelit
Copy link
Contributor Author

lelit commented Jun 20, 2017

Thank you, have a good time!

elprans added a commit that referenced this issue Jul 4, 2017
asyncpg will now treat 12-month interval increments as 365-day spans,
instead of 12*30 day spans.  This matches psycopg2 interval decoding
logic.

Fixes: #150.
@elprans elprans closed this as completed in 6b48443 Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants