Please address the comments from this code review:

## Overall Comments
- In `MergeMaxByAggregations.PATTERN` you detect candidates via `getCall().getDisplayName().equals("max_by")`, while the main logic uses `functionResolution.isMaxByFunction(...)`; consider using the same function-resolution-based check in the pattern to avoid missing or misidentifying max_by calls (e.g., if naming or resolution changes).
- The `MaxByKey` grouping key currently considers only the comparison expression, filter, mask, and distinct flag; if `Aggregation.getOrderBy()` is non-empty, merging aggregations with different ORDER BY clauses could change semantics, so it may be safer to include `orderBy` (and any other behavior-affecting properties) in `MaxByKey.equals`/`hashCode`.
- The session property registration for `ADD_DISTINCT_BELOW_SEMI_JOIN_BUILD` was changed to use the four-argument `booleanProperty(..., false)` overload when adding `MERGE_MAX_BY_AGGREGATIONS`; double-check that the visibility/experimental flags for the existing property are intentional and unchanged in behavior.

## Individual Comments

### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/iterative/rule/MergeMaxByAggregations.java:70-77` </location>
<code_context>
+public class MergeMaxByAggregations
+        implements Rule<AggregationNode>
+{
+    private static final String MAX_BY = "max_by";
+
+    private final FunctionResolution functionResolution;
+
+    private static final Pattern<AggregationNode> PATTERN = aggregation()
+            .matching(MergeMaxByAggregations::hasMultipleMaxBy);
+
+    private static boolean hasMultipleMaxBy(AggregationNode aggregationNode)
+    {
+        return aggregationNode.getAggregations().values().stream()
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Avoid relying on displayName and a hard-coded string to detect MAX_BY in the pattern predicate.

The pattern uses `agg.getCall().getDisplayName().equals(MAX_BY)` while `apply()` uses `functionResolution.isMaxByFunction(...)`. This mismatch can cause the pattern to fire differently than the logic in `apply()` (e.g., with case, qualification, or future renames). Please align the predicate with the function-resolution-based approach (e.g., use the function handle rather than the display name) so the match condition and core logic stay in sync.

Suggested implementation:

```java
    private final FunctionResolution functionResolution;
    private final Pattern<AggregationNode> pattern;

    private boolean hasMultipleMaxBy(AggregationNode aggregationNode)
    {
        return aggregationNode.getAggregations().values().stream()
                .filter(agg -> functionResolution.isMaxByFunction(agg.getCall().getFunctionHandle()))
                .count() > 1;
    }

    public MergeMaxByAggregations(FunctionAndTypeManager functionAndTypeManager)
    {
        requireNonNull(functionAndTypeManager, "functionAndTypeManager is null");
        this.functionResolution = new FunctionResolution(functionAndTypeManager.getFunctionAndTypeResolver());
        this.pattern = aggregation()
                .matching(this::hasMultipleMaxBy);
    }

```

There is likely a `getPattern()` method later in the class that currently returns the static `PATTERN` field. That method should be updated to return the new instance field instead:

1. Change `return PATTERN;` to `return pattern;` in `getPattern()`.
2. Remove the now-unused static `PATTERN` field declaration if it appears elsewhere in the file.
</issue_to_address>

### Comment 2
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/iterative/rule/MergeMaxByAggregations.java:255-257` </location>
<code_context>
+    private static class MaxByKey
+    {
+        private final RowExpression comparisonKey;
+        private final Optional<RowExpression> filter;
+        private final Optional<VariableReferenceExpression> mask;
+        private final boolean isDistinct;
+
+        public MaxByKey(Aggregation aggregation)
</code_context>

<issue_to_address>
**issue (bug_risk):** Consider including ORDER BY in MaxByKey to avoid merging aggregations with different ordering.

MaxByKey currently keys aggregations by comparisonKey, filter, mask, and distinct, but not `Aggregation.getOrderBy()`. The merged aggregation then uses `firstAggregation.getOrderBy()`. If two MAX_BY aggregations share those fields but differ in ORDER BY, they will be merged and all will effectively use the first aggregation’s ORDER BY, changing semantics. If this can occur, either add orderBy to MaxByKey’s key fields or explicitly reject non-empty/differing ORDER BY to avoid incorrect rewrites.
</issue_to_address>

### Comment 3
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/iterative/rule/MergeMaxByAggregations.java:104-106` </location>
<code_context>
+    @Override
+    public Result apply(AggregationNode node, Captures captures, Context context)
+    {
+        Map<VariableReferenceExpression, Aggregation> maxByAggregations = node.getAggregations().entrySet().stream()
+                .filter(entry -> isMaxByAggregationWithTwoArgs(entry.getValue()))
+                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+        if (maxByAggregations.size() < 2) {
</code_context>

<issue_to_address>
**suggestion:** Use a map implementation that preserves insertion order explicitly instead of relying on Collectors.toMap defaults.

This stream currently collects into a default `HashMap` via `Collectors.toMap`, so the iteration order is unspecified. Since `groupedByKey` is later a `LinkedHashMap` to keep grouping order, consider collecting directly into a `LinkedHashMap` (e.g., `Collectors.toMap(..., LinkedHashMap::new, (a, b) -> a)`) or using `ImmutableMap.toImmutableMap` to ensure deterministic ordering and plan stability.

```suggestion
        Map<VariableReferenceExpression, Aggregation> maxByAggregations = node.getAggregations().entrySet().stream()
                .filter(entry -> isMaxByAggregationWithTwoArgs(entry.getValue()))
                .collect(Collectors.toMap(
                        Map.Entry::getKey,
                        Map.Entry::getValue,
                        (first, second) -> first,
                        LinkedHashMap::new));
```
</issue_to_address>

### Comment 4
<location> `presto-main-base/src/test/java/com/facebook/presto/sql/planner/iterative/rule/TestMergeMaxByAggregations.java:42-51` </location>
<code_context>
+                                .withAlias("max_v3", dereferenceInRange("max_by", 0, 2)));
+    }
+
+    @Test
+    public void testDoesNotFireWithSingleMaxBy()
+    {
+        tester().assertThat(new MergeMaxByAggregations(getMetadata().getFunctionAndTypeManager()))
+                .setSystemProperty(MERGE_MAX_BY_AGGREGATIONS, "true")
+                .on(p -> p.aggregation(af -> {
+                    p.variable("v1", BIGINT);
+                    p.variable("k", BIGINT);
+                    af.globalGrouping()
+                            .addAggregation(p.variable("max_v1"), p.rowExpression("max_by(v1, k)"))
+                            .source(p.values(p.variable("v1"), p.variable("k")));
+                }))
+                .doesNotFire();
+    }
+
</code_context>

<issue_to_address>
**suggestion (testing):** Add a positive-plan test for merging DISTINCT MAX_BY aggregations

You already have a negative test (`testDoesNotMergeMaxByWithDistinct`) for mixed DISTINCT/non-DISTINCT `MAX_BY`, which is good. Since the rule also groups by `isDistinct` in `MaxByKey`, please add a positive test where both `MAX_BY` aggregations are DISTINCT, share the same comparison key and filter, and are verified via `PlanMatchPattern` to merge into a single DISTINCT `max_by(ROW(...), key)` aggregation. This will help catch regressions where the DISTINCT flag is mishandled in the all-DISTINCT case.

Suggested implementation:

```java
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.aggregation;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.column;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.globalAggregation;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.node;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.project;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.rowConstructor;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.singleGroupingSet;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.values;

```

```java
     @Test
     public void testDoesNotFireWithSingleMaxBy()
     {
         tester().assertThat(new MergeMaxByAggregations(getMetadata().getFunctionAndTypeManager()))
                 .setSystemProperty(MERGE_MAX_BY_AGGREGATIONS, "true")
                 .on(p -> p.aggregation(af -> {
                     p.variable("v1", BIGINT);
                     p.variable("k", BIGINT);
                     af.globalGrouping()
                             .addAggregation(p.variable("max_v1"), p.rowExpression("max_by(v1, k)"))
                             .source(p.values(p.variable("v1"), p.variable("k")));
                 }))
                 .doesNotFire();
     }

     @Test
     public void testMergesDistinctMaxByWithSameKeyAndFilter()
     {
         tester().assertThat(new MergeMaxByAggregations(getMetadata().getFunctionAndTypeManager()))
                 .setSystemProperty(MERGE_MAX_BY_AGGREGATIONS, "true")
                 .on(p -> p.aggregation(af -> {
                     p.variable("v1", BIGINT);
                     p.variable("v2", BIGINT);
                     p.variable("k", BIGINT);
                     p.variable("f", BOOLEAN);

                     af.globalGrouping()
                             // both DISTINCT, same key and same filter
                             .addAggregation(p.variable("max_v1"), p.rowExpression("max_by(DISTINCT v1, k) FILTER (WHERE f)"))
                             .addAggregation(p.variable("max_v2"), p.rowExpression("max_by(DISTINCT v2, k) FILTER (WHERE f)"))
                             .source(p.values(p.variable("v1"), p.variable("v2"), p.variable("k"), p.variable("f")));
                 }))
                 .matches(
                         // Expect a single DISTINCT max_by over a ROW(v1, v2) and key k
                         project(
                                 node(AggregationNode.class,
                                         globalAggregation(
                                                 singleGroupingSet(),
                                                 ImmutableMap.of(
                                                         Optional.empty(),
                                                         aggregation(
                                                                 "max_by",
                                                                 ImmutableList.of(
                                                                         rowConstructor(column("v1"), column("v2")),
                                                                         column("k")))
                                                                 .distinct()))))));
     }


```

Depending on the exact PlanMatchPattern APIs and existing conventions in this test class, you might need to:
1. Adjust the `singleGroupingSet()` call in the matcher to explicitly specify the grouping key symbol (for example, `singleGroupingSet("k")` or `singleGroupingSet(column("k"))`) to match how other tests in this file describe grouping sets.
2. Confirm that `Optional.empty()` is an acceptable key in the `ImmutableMap` used by `globalAggregation`. If other tests key aggregations by symbol name (e.g., `Optional.of("max")`), align with that convention and use the appropriate alias.
3. If other tests use a different style for plan matching (for example, they use `aggregation(...)` directly inside `node(AggregationNode.class, ...)` without `globalAggregation(...)`), rewrite the `.matches(...)` section to mirror that pattern while still asserting:
   - there is exactly one `max_by` aggregation,
   - its first argument is `ROW(v1, v2)`,
   - its second argument is `k`,
   - and it is `.distinct()`.
These tweaks will make the new positive DISTINCT test consistent with the rest of the file and ensure it remains robust against minor plan-shape differences.
</issue_to_address>