From 826389870d2dafb863efcf2f0d3cd32c7ec1f685 Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Wed, 22 May 2024 09:53:11 +0000
Subject: [PATCH] perf: Avoid coalesce for `between` filters (#26531) (#26532)

- Avoid on `between` + date
- Avoid on timestamp fields
- Avoid on `>` and `>=` comparisons

(cherry picked from commit 005e74b20d5133e3a76d29285596e31921bc495a)

Co-authored-by: Ankush Menat <ankush@frappe.io>
---
 frappe/model/db_query.py      | 23 +++++++++++++++++++++--
 frappe/tests/test_db_query.py |  9 +++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py
index 011214fadbe..88553495967 100644
--- a/frappe/model/db_query.py
+++ b/frappe/model/db_query.py
@@ -723,7 +723,7 @@ def prepare_filter_condition(self, f):
 		meta = frappe.get_meta(f.doctype)
 
 		# primary key is never nullable, modified is usually indexed by default and always present
-		can_be_null = f.fieldname not in ("name", "modified")
+		can_be_null = f.fieldname not in ("name", "modified", "creation")
 
 		# prepare in condition
 		if f.operator.lower() in NestedSetHierarchy:
@@ -802,12 +802,20 @@ def prepare_filter_condition(self, f):
 
 			if f.operator.lower() in ("previous", "next", "timespan"):
 				date_range = get_date_range(f.operator.lower(), f.value)
-				f.operator = "Between"
+				f.operator = "between"
 				f.value = date_range
 				fallback = f"'{FallBackDateTimeStr}'"
 
+			if f.operator.lower() in (">", ">=") and (
+				f.fieldname in ("creation", "modified")
+				or (df and (df.fieldtype == "Date" or df.fieldtype == "Datetime"))
+			):
+				# Null values can never be greater than any non-null value
+				can_be_null = False
+
 			if f.operator in (">", "<", ">=", "<=") and (f.fieldname in ("creation", "modified")):
 				value = cstr(f.value)
+				can_be_null = False
 				fallback = f"'{FallBackDateTimeStr}'"
 
 			elif f.operator.lower() in ("between") and (
@@ -815,6 +823,17 @@ def prepare_filter_condition(self, f):
 				or (df and (df.fieldtype == "Date" or df.fieldtype == "Datetime"))
 			):
 				escape = False
+				# Between operator never needs to check for null
+				# Explanation: Consider SQL -> `COLUMN between X and Y`
+				# Actual computation:
+				#     for row in rows:
+				#     if Y > row.COLUMN > X:
+				#         yield row
+
+				# Since Y and X can't be null, null value in column will never match filter, so
+				# coalesce is extra cost that prevents index usage
+				can_be_null = False
+
 				value = get_between_date_filter(f.value, df)
 				fallback = f"'{FallBackDateTimeStr}'"
 
diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py
index f3eb2c8ea3b..09fb3ee47c7 100644
--- a/frappe/tests/test_db_query.py
+++ b/frappe/tests/test_db_query.py
@@ -1112,6 +1112,15 @@ def test_coalesce_with_in_ops(self):
 		self.assertNotIn("ifnull", frappe.get_all("User", {"name": ("in", (""))}, run=0))
 		self.assertNotIn("ifnull", frappe.get_all("User", {"name": ("in", ())}, run=0))
 
+	def test_coalesce_with_datetime_ops(self):
+		self.assertNotIn("ifnull", frappe.get_all("User", {"last_active": (">", "2022-01-01")}, run=0))
+		self.assertNotIn("ifnull", frappe.get_all("User", {"creation": ("<", "2022-01-01")}, run=0))
+		self.assertNotIn(
+			"ifnull",
+			frappe.get_all("User", {"last_active": ("between", ("2022-01-01", "2023-01-01"))}, run=0),
+		)
+		self.assertIn("ifnull", frappe.get_all("User", {"last_active": ("<", "2022-01-01")}, run=0))
+
 	def test_ambiguous_linked_tables(self):
 		from frappe.desk.reportview import get