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

Updates #115

Merged
merged 5 commits into from
Oct 15, 2020
Merged

Updates #115

merged 5 commits into from
Oct 15, 2020

Conversation

Photonios
Copy link
Member

No description provided.

ratoma and others added 5 commits October 15, 2020 18:06
If only a subset of annotations is renamed before calling the base class,
and then renamed back using add new key/remove old key, then the ordering of
annotations is changed (annotations are stored in a OrderedDict internally).
This will cause issues if ordering is important (e.g., in a union).
@Photonios Photonios merged commit 3bb20c2 into master Oct 15, 2020
@warmwaffles
Copy link

Hey @Photonios this commit b70d239 breaks my application. I have to stop using the default manager because it aliased _new to an annotated column that I put in.

@Photonios
Copy link
Member Author

Thanks for the heads up! I am going to figure out a fix or roll it back in the next release somehow.

@warmwaffles
Copy link

warmwaffles commented Oct 20, 2020

@Photonios here is a brief example

class SomeReport(PostgresPartitionedModel):
    class PartitioningMeta:
        method = PostgresPartitioningMethod.RANGE
        key = ["report_date"]

    id = models.UUIDField(primary_key=True, default=uuid.uuid4)
    account = models.ForeignKey(to=Account, on_delete=models.DO_NOTHING)
    report_date = models.DateField()
    metric = models.IntegerField()

# ...

result = AmsCampaignReport.objects.filter(
    account__in=some_set_of_accounts,
    report_date__range=(start, stop)
).values(
    "account_id",
    date=F("report_date")
).annotate(
    total_metric=Sum(F("metric"))
).all()

# {"account_id": xxxx, "date": "2020-10-01", "total_metric_new": 123}
# vs before this used to be
# {"account_id": xxxx, "date": "2020-10-01", "total_metric": 123}

@Photonios
Copy link
Member Author

Hi!

Thanks for the example. Could you tell me the Django and Postgres version you're using?

I also noticed that in your example: total_metric=Sum(F("cost")) cost is not defined anywhere.

@warmwaffles
Copy link

@Photonios We are using postgres 11 and django 3.0

I've updated the example. Sorry it was some cut and paste issue that the cost snuck in.

@Photonios Photonios deleted the updates branch October 24, 2020 10:53
@Photonios
Copy link
Member Author

@warmwaffles Sorry for the delay on this.

I've reproduced your example as a test, but I cannot confirm the issue you're describing: #116

The tests pass on all Python and Django versions (from Python 3.5 to 3.9, Django 2.0 to 3.1).

Could you show me how to adjust the test I linked to reproduce the problem you're describing?

@DanielAlderman
Copy link

DanielAlderman commented Oct 30, 2020

@Photonios I am getting a similar issue as @warmwaffles, but agree with you that it isn't always present.

I'm running Postgres 10.14 and Django 3.1.

In my experience this bug happens when I introduce a Case object.

(Edited to make it more readable.)

class MainJobDetail(models.Model):
    required_time = models.DateTimeField()

with postgres_manager(MainJobDetail) as mjd:
    working = mjd.annotate(
        hour = ExtractHour('required_time')
    ).first()

    print(working.hour)

    not_working = mjd.annotate(
        hour = ExtractHour('required_time'),
        slot_order = Case(
            When(
                Q(hour__lt = 7),
                then = Value('0')
            ),
            default = Value('1'),
            output_field = CharField()
        )
    ).first()

    print(not_working.hour)

I get the value as intended for working, but I get django.core.exceptions.FieldError: Cannot resolve keyword 'hour' into field. Choices are: hour_new, required_time on not_working

I hope this makes sense and helps you to replicate the issue.

@Photonios
Copy link
Member Author

Photonios commented Oct 30, 2020

Hi @DanielAlderman! Thanks for the extra details.

Could you post the contents of the ExtractHour annotation? I suspect that might have something to do with it.

EDIT: Nevermind, ExtractHour is standard Django. Trying to reproduce it now.

@Photonios
Copy link
Member Author

Photonios commented Oct 30, 2020

@warmwaffles @DanielAlderman With the help of @DanielAlderman 's example, I managed to reproduce the issue and I've fixed it in this commit: 96fd08b

I've released this fix in v2.0.3rc1, on both Github and PyPi: https://github.com/SectorLabs/django-postgres-extra/releases/tag/v2.0.3rc1

Please give this version a shot and let me know if you still have trouble.

@DanielAlderman
Copy link

Hi @Photonios

Thanks for looking at this so quickly, I've installed v.2.0.3rc1 and can confirm I am no longer getting the error.

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.

None yet

5 participants