Skip to content

Fixed #24767 -- Added Greatest and Least expressions#4634

Closed
LilyFirefly wants to merge 1 commit intodjango:masterfrom
LilyFirefly:greatest-least-expressions
Closed

Fixed #24767 -- Added Greatest and Least expressions#4634
LilyFirefly wants to merge 1 commit intodjango:masterfrom
LilyFirefly:greatest-least-expressions

Conversation

@LilyFirefly
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greatest() should do.

@LilyFirefly
Copy link
Copy Markdown
Contributor Author

This is still failing on MySQL. It appears that now the annotated values are not being converted back into the correct python types. Actually, this seems to have been a build fluke.

@LilyFirefly
Copy link
Copy Markdown
Contributor Author

What do I still need to do for this? I know documentation is needed. Do I need to add any more tests, for example for other databases?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Mysql, Oracle, and SQLite

@jarshwah
Copy link
Copy Markdown
Member

You still need to implement Least (which is just the reverse of Greatest). I think the duality means both should ship in the same commit.

I'd also like to see some tests that used related fields:

.annotate(eldest_friend=Greatest('friends', 'friends__age'))

The tests should cover non-null and null relations to have the best coverage.

I'd also like to see a test that uses Greatest for updating an existing field:

.update(last_updated=Greatest('comments__datetime', 'last_updated'))

I'm using made up fields above, but there should be models available that you can find a use case for. If not, it's ok to create a new model that fits the need.

Once that's all done (and docs are written etc) it'd be good if you could squash all the commits into one, to make it easier on whoever will merge the PR. But we'll review some more before that's necessary anyway.

@LilyFirefly
Copy link
Copy Markdown
Contributor Author

Yeah, I was waiting to do Least until everything had been worked out with Greatest.

@LilyFirefly LilyFirefly force-pushed the greatest-least-expressions branch 2 times, most recently from 4ea9d5d to ef625ec Compare May 12, 2015 20:15
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jarshwah Is this the sort of test you were thinking of?
It currently fails with a FieldError that looks related to #14104.

Traceback (most recent call last):
  File "/home/ian/dev/django/tests/db_functions/tests.py", line 164, in test_greatest_update
    Author.objects.update(age=Greatest('age', 'fans__age'))
  File "/home/ian/dev/django/django/db/models/manager.py", line 125, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/ian/dev/django/django/db/models/query.py", line 619, in update
    rows = query.get_compiler(self.db).execute_sql(CURSOR)
  File "/home/ian/dev/django/django/db/models/sql/compiler.py", line 1065, in execute_sql
    cursor = super(SQLUpdateCompiler, self).execute_sql(result_type)
  File "/home/ian/dev/django/django/db/models/sql/compiler.py", line 832, in execute_sql
    sql, params = self.as_sql()
  File "/home/ian/dev/django/django/db/models/sql/compiler.py", line 1018, in as_sql
    val = val.resolve_expression(self.query, allow_joins=False, for_save=True)
  File "/home/ian/dev/django/django/db/models/expressions.py", line 491, in resolve_expression
    c.source_expressions[pos] = arg.resolve_expression(query, allow_joins, reuse, summarize, for_save)
  File "/home/ian/dev/django/django/db/models/expressions.py", line 448, in resolve_expression
    return query.resolve_ref(self.name, allow_joins, reuse, summarize)
  File "/home/ian/dev/django/django/db/models/sql/query.py", line 1416, in resolve_ref
    raise FieldError("Joined field references are not permitted in this query")
django.core.exceptions.FieldError: Joined field references are not permitted in this query

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it was Ian, but my example used joined fields for an update which isn't (yet) allowed.

How about something like:

Author.objects.update(alias=Greatest('name', 'goes_by')

Which will also test the handling of varchars in a Greatest.

@timgraham timgraham changed the title Add Greatest and Least expressions Fixed #24767 -- Added Greatest and Least expressions May 12, 2015
@LilyFirefly LilyFirefly force-pushed the greatest-least-expressions branch from 20ca258 to 3ae92dc Compare June 4, 2015 10:05
@mjtamlyn mjtamlyn force-pushed the greatest-least-expressions branch from a004119 to ea41273 Compare June 4, 2015 11:29
@mjtamlyn
Copy link
Copy Markdown
Member

mjtamlyn commented Jun 4, 2015

buildbot, test on oracle.

@mjtamlyn
Copy link
Copy Markdown
Member

mjtamlyn commented Jun 4, 2015

Looks like your coalesce workaround fails on MySQL. Good news is Oracle is fine.

Greatest and Least are row-level Function versions of Min and Max.
@LilyFirefly LilyFirefly force-pushed the greatest-least-expressions branch from ea41273 to c629a01 Compare June 4, 2015 16:19
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #24925.

@LilyFirefly
Copy link
Copy Markdown
Contributor Author

It looks like the buildbot didn't pick up my most recent change (adding test_greatest_coalesce_workaround_mysql and test_least_coalesce_workaround_mysql).

@mjtamlyn
Copy link
Copy Markdown
Member

mjtamlyn commented Jun 5, 2015

buildbot, test this please

@mjtamlyn
Copy link
Copy Markdown
Member

mjtamlyn commented Jun 5, 2015

Merged in 4ab53a5

@mjtamlyn mjtamlyn closed this Jun 5, 2015
@LilyFirefly LilyFirefly deleted the greatest-least-expressions branch June 5, 2015 10:24
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.

5 participants