Skip to content
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

Unexpected column names when using fn and .dicts #2305

Closed
wants to merge 1 commit into from
Closed

Unexpected column names when using fn and .dicts #2305

wants to merge 1 commit into from

Conversation

albatros69
Copy link

@albatros69 albatros69 commented Nov 26, 2020

Consider the following example (I tried to shave it to a minimum):

class Test(Model):
    start_date = DateTimeField(null=True)
    end_date = DateTimeField(null=True)

>>> list(Test.select(fn.MIN(Test.start_date), fn.MAX(Test.end_date)).dicts())
[{'start_date")': datetime.datetime(2020, 9, 11, 9, 56), 'end_date")': datetime.datetime(2020, 11, 29, 16, 0)}]

See the extra quote and parenthesis for the column names. One would expect something more like:

>>> list(Test.select(fn.MIN(Test.start_date), fn.MAX(Test.end_date)).dicts())
[{'start_date': datetime.datetime(2020, 9, 11, 9, 56), 'end_date': datetime.datetime(2020, 11, 29, 16, 0)}]

I'm just proposing here to strip the colum names from the extra right parenthesis.

@coleifer
Copy link
Owner

I'm OK with adding this, but you should really be explicitly aliasing these so that it is not database-dependent and fragile. I believe, furthermore, this example is specific to sqlite - postgres behaves differently (I believe the columns will be named "min" and "max", at least that's how it behaves on my laptop with postgres 13), and older versions of sqlite behave even differently if I recall correctly.

I've implemented a similar patch which also adds a test-case and merged.

@coleifer coleifer closed this Nov 26, 2020
coleifer added a commit that referenced this pull request Nov 26, 2020
This seems to only apply when using Sqlite. Postgres is using the name
of the function, for example.

Refs #2305
@albatros69
Copy link
Author

Thanks a lot for the answer. You're absolutely right about the aliasing necessity. I discovered this issue as part of some quick&dirty debugging, where i didn't take the time to alias (which I usually do). As it was quite straight forward to correct, I thought it was better to consolidate that in a PR.

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

Successfully merging this pull request may close these issues.

2 participants