Greatest() argument count guards do not align with certain database backends

[Martijn Verkleij]

I ran into two issues while I tried to use this comparison function django.db.models.function.comparison.Greatest:

• It forces you to use two or more arguments, while the Postgres database backend, for example, allows for one argument.
• If the argument supplied is a generator expression, it is not unpacked and the above check triggers while more arguments are supplied.

I found these while using Django 4.2, but I have checked the current source code and it is unchanged compared to that version.

For the first one, a possible solution might be to move the check to the different database backends, or be generalized somehow to allow certain backends to have different numbers of arguments. One could even consider writing custom SQL generators for lists of length one (returning the value bare), and an exception for passing the empty list.

The second one appears to be a Python language limitation for which it is debatable whether to bother implementing anything to fix it. Converting the expression to a tuple and unpacking it solves the problem.

[Simon Charette]

Thank you for your report but I don't this that these should be considered as bugs.

It forces you to use two or more arguments, while the Postgres database backend, for example, allows for one argument.
...
For the first one, a possible solution might be to move the check to the different database backends, or be generalized somehow to allow certain backends to have different numbers of arguments. One could even consider writing custom SQL generators for lists of length one (returning the value bare), and an exception for passing the empty list.

The expressions provided by Django's core ought to be good compromise between all backends it supports and these objects are backend agnostic until they are actually compiled for execution. Given it's easy enough to write a simple function of the form

def greatest_expr(*exprs):
    if len(exprs) == 1:
        return exprs[0]
    return Greatest(*exprs)

I'm not entirely convinced adjusting Greatest to do the same but at resolving time is worth doing even if it's relatively trivial to adjust Greatest.resolve_expression to accomplish the same (resolve to the first provided expression if there's a single one which is backend agnostic). I would qualify this as a new feature though.

django/db/models/functions/comparison.py

@@ -131 +131 @@
     function = "GREATEST"
 
     def __init__(self, *expressions, **extra):
-        if len(expressions) < 2:
-            raise ValueError("Greatest must take at least two expressions")
+        if len(expressions) < 1:
+            raise ValueError("Greatest must take at least one expression")
         super().__init__(*expressions, **extra)
 
+    def resolve_expression(self, *args, **kwargs):
+        resolved = super().resolve_expression(*args, **kwargs)
+        if len(source_expressions := resolved.get_source_expressions()) == 1:
+            return source_expressions[0]
+        return resolved
+
     def as_sqlite(self, compiler, connection, **extra_context):
         """Use the MAX function on SQLite."""
         return super().as_sqlite(compiler, connection, function="MAX", **extra_context)

If the argument supplied is a generator expression, it is not unpacked and the above check triggers while more arguments are supplied.
...
The second one appears to be a Python language limitation for which it is debatable whether to bother implementing anything to fix it. Converting the expression to a tuple and unpacking it solves the problem.

I'm to assume you meant that Greatest(expr in expr_generator) was not automatically picked up as Greatest(*(expr in expr_generator))? The signature of expressions, Greatest in this case, are not defined as (Iterable[Expression]) but *Expression so you must ensure to call them in this fashion. I don't see it as a limitation of the Python language but simply as an improper function call.

[Martijn Verkleij]

Hello,

Thanks for the quick response!

I think you are fair in your assessment of the problem. Calling this function with a variable, non-explicit amount of arguments without the user of the function programming defensively can only be recipe for disaster anyway. Whether it is worth doing this for the few backends that support a single argument for this function is doubtful with that in mind.

Anyway, if a generalization like you suggest would be implemented, it could maybe work for LEAST and COALESCE too as they similarly only require one argument.

I'm to assume you meant that Greatest(expr in expr_generator) was not automatically picked up as Greatest(*(expr in expr_generator))? The signature of expressions, Greatest in this case, are not defined as (Iterable[Expression]) but *Expression so you must ensure to call them in this fashion. I don't see it as a limitation of the Python language but simply as an improper function call.

That was indeed what I meant. I was already afraid this was a case of "user error". I did not realise I could have seen this coming through the method signature, totally forgetting that Iterable[] exists and that this is how packing/unpacking works. Thanks, I learned something new today!

[Simon Charette]

I agree that if we decide this change to be worth it we should likely use the same approach for Least and Coalesce but I remain unconvinced that it's worth adding as it's pretty simple not to use these functions when dealing with a single expression.

Maybe you could try testing the water on the forum to see if other members of the community ran into a similar edge case with singleton expression wrapping? ​That's the usual process for requesting features anyway.

[Sarah Boyce]

Hi, thank you both for the ticket and the discussion

I agree that this is a new feature. As Simon mentioned, the recommended path forward is to first propose and discuss the idea with the community and gain consensus. To do that, please consider starting a new conversation on the ​Django Forum, where you'll reach a broader audience and receive additional feedback.

I'll close the ticket for now, but if the community agrees with the proposal, please return to this ticket and reference the forum discussion so we can re-open it. For more information, please refer to ​the documented guidelines for requesting features.