Repository: duckdb/duckdb (language: python)

Issue:
max_by/min_by() new behavior - feature or bug?
### What happens?

The behavior of min_by() and max_by() has changed in the new 1.1.0 release. `max_by(a, b)` seems to now return `a` if there is a `null` in `b`, instead of returning `null` like in the previous version (I tested at least until 0.8.1). Returning null is the behavior of other engines like Presto (and perhaps the most intuitive for users?)

Was the change a feature or a bug? (please close in case the change was intentional)

### To Reproduce

```
pedroerp@fedora:~/Github/duck-0.8.1$ ./duckdb 
v0.8.1 6536a77232
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
```
```sql
SELECT max_by(c0, c1) FROM (values (1, null)) t(c0,c1);
```
```
┌────────────────┐
│ max_by(c0, c1) │
│     int32      │
├────────────────┤
│                │
└────────────────┘
```
```
pedroerp@fedora:~/Github/duck-0.8.1$ cd ../duck-1.1.0/
pedroerp@fedora:~/Github/duck-1.1.0$ ./duckdb 
v1.1.0 fa5c2fe15f
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
```
```sql
SELECT max_by(c0, c1) FROM (values (1, null)) t(c0,c1);
```
```
┌────────────────┐
│ max_by(c0, c1) │
│     int32      │
├────────────────┤
│              1 │
└────────────────┘
```

### OS:

Fedora

### DuckDB Version:

1.1.0 and 0.8.1

### DuckDB Client:

Command line client

### Hardware:

_No response_

### Full Name:

Pedro Pedreira

### Affiliation:

Meta

### What is the latest build you tested with? If possible, we recommend testing with the latest nightly build.

I have tested with a stable release

### Did you include all relevant data sets for reproducing the issue?

Yes

### Did you include all code required to reproduce the issue?

- [X] Yes, I have

### Did you include all relevant configuration (e.g., CPU architecture, Python version, Linux distribution) to reproduce the issue?

- [X] Yes, I have
max_by/min_by() new behavior - feature or bug?
### What happens?

The behavior of min_by() and max_by() has changed in the new 1.1.0 release. `max_by(a, b)` seems to now return `a` if there is a `null` in `b`, instead of returning `null` like in the previous version (I tested at least until 0.8.1). Returning null is the behavior of other engines like Presto (and perhaps the most intuitive for users?)

Was the change a feature or a bug? (please close in case the change was intentional)

### To Reproduce

```
pedroerp@fedora:~/Github/duck-0.8.1$ ./duckdb 
v0.8.1 6536a77232
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
```
```sql
SELECT max_by(c0, c1) FROM (values (1, null)) t(c0,c1);
```
```
┌────────────────┐
│ max_by(c0, c1) │
│     int32      │
├────────────────┤
│                │
└────────────────┘
```
```
pedroerp@fedora:~/Github/duck-0.8.1$ cd ../duck-1.1.0/
pedroerp@fedora:~/Github/duck-1.1.0$ ./duckdb 
v1.1.0 fa5c2fe15f
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
```
```sql
SELECT max_by(c0, c1) FROM (values (1, null)) t(c0,c1);
```
```
┌────────────────┐
│ max_by(c0, c1) │
│     int32      │
├────────────────┤
│              1 │
└────────────────┘
```

### OS:

Fedora

### DuckDB Version:

1.1.0 and 0.8.1

### DuckDB Client:

Command line client

### Hardware:

_No response_

### Full Name:

Pedro Pedreira

### Affiliation:

Meta

### What is the latest build you tested with? If possible, we recommend testing with the latest nightly build.

I have tested with a stable release

### Did you include all relevant data sets for reproducing the issue?

Yes

### Did you include all code required to reproduce the issue?

- [X] Yes, I have

### Did you include all relevant configuration (e.g., CPU architecture, Python version, Linux distribution) to reproduce the issue?

- [X] Yes, I have


Discussion / hints:

Fix patch:
diff --git a/src/core_functions/aggregate/distributive/arg_min_max.cpp b/src/core_functions/aggregate/distributive/arg_min_max.cpp
index 5120bc797cd9..be64b065f509 100644
--- a/src/core_functions/aggregate/distributive/arg_min_max.cpp
+++ b/src/core_functions/aggregate/distributive/arg_min_max.cpp
@@ -192,7 +192,7 @@ struct GenericArgMinMaxState {
 
 	static void PrepareData(Vector &by, idx_t count, Vector &extra_state, UnifiedVectorFormat &result) {
 		OrderModifiers modifiers(ORDER_TYPE, OrderByNullType::NULLS_LAST);
-		CreateSortKeyHelpers::CreateSortKey(by, count, modifiers, extra_state);
+		CreateSortKeyHelpers::CreateSortKeyWithValidity(by, extra_state, modifiers, count);
 		extra_state.ToUnifiedFormat(count, result);
 	}
 };
diff --git a/src/include/duckdb/core_functions/aggregate/minmax_n_helpers.hpp b/src/include/duckdb/core_functions/aggregate/minmax_n_helpers.hpp
index 09b0bb7d98d0..6dbdb8bce800 100644
--- a/src/include/duckdb/core_functions/aggregate/minmax_n_helpers.hpp
+++ b/src/include/duckdb/core_functions/aggregate/minmax_n_helpers.hpp
@@ -1,7 +1,14 @@
 #pragma once
 
-#include "duckdb/common/vector.hpp"
+#include "duckdb/common/common.hpp"
+#include "duckdb/storage/arena_allocator.hpp"
 #include "duckdb/common/algorithm.hpp"
+#include "duckdb/common/pair.hpp"
+#include "duckdb/common/types/string_type.hpp"
+#include "duckdb/common/types/vector.hpp"
+#include "duckdb/common/enums/order_type.hpp"
+#include "duckdb/function/aggregate_function.hpp"
+#include "duckdb/core_functions/create_sort_key.hpp"
 
 namespace duckdb {
 
@@ -294,11 +301,8 @@ struct MinMaxFallbackValue {
 
 	static void PrepareData(Vector &input, const idx_t count, EXTRA_STATE &extra_state, UnifiedVectorFormat &format) {
 		const OrderModifiers modifiers(OrderType::ASCENDING, OrderByNullType::NULLS_LAST);
-		CreateSortKeyHelpers::CreateSortKey(input, count, modifiers, extra_state);
+		CreateSortKeyHelpers::CreateSortKeyWithValidity(input, extra_state, modifiers, count);
 		input.Flatten(count);
-		extra_state.Flatten(count);
-		// Ensure the validity vectors match, because we want to ignore nulls
-		FlatVector::Validity(extra_state).Initialize(FlatVector::Validity(input));
 		extra_state.ToUnifiedFormat(count, format);
 	}
 };


Test patch:
diff --git a/test/sql/aggregate/aggregates/test_arg_min_max.test b/test/sql/aggregate/aggregates/test_arg_min_max.test
index 23fa937af1ac..d7781a51116d 100644
--- a/test/sql/aggregate/aggregates/test_arg_min_max.test
+++ b/test/sql/aggregate/aggregates/test_arg_min_max.test
@@ -193,3 +193,8 @@ FROM employees;
 1030
 
 endloop
+
+query I
+SELECT max_by(c0, c1) FROM (values (1, null)) t(c0,c1);
+----
+NULL