Avoid a case where the planner was erroneously dropping predicates

[alecgrieser]

There was a case in RangeConstraints.asComparisonRange where the process of merging different comparison ranges could result in predicates being dropped. The fundamental error was that there is a function, merge, which merges a comparison into a comparison range. However, if the comparison can't be successfully merged, then it will be added to a residual comparison list. But the asComparisonRange function was ignoring those predicates entirely.

This modifies the method so that it collects the ranges rather than dropping them. It then also modifies the compensation function so that if there are residual predicates, we are sure to generate a predicate for it.

This fixes #3950.

[RobertBrunel]

A side observation related to the getRangeType() comment below — I wonder whether we should just check getRangeType() here as well. What happens if one of the two is isEmpty()? There could be another bug lurking here.

[alecgrieser]

It's possible... I'd rather we do this in a separate PR, though, as we're kind of far afield from the core bug fix at this point

[RobertBrunel]

OK

[alecgrieser]

I've addressed some of (but not all of) the original comments. This mainly has involved doing additional refactoring/method extraction. I'm curious if (1) this looks more readable to you and (2) if the extractions look correct. I think there is one issue that I'm still not sure about, which is the thing you mentioned about basing the compensation off of different base predicates in different paths, and I'm still not sure if that was just an error on my part or not. In the new code, they now share a common function for that, so they're basing their compensation off of the same predicate

[alecgrieser]

I've merged in some recent metrics changes and then resolved some additional comments to try and make the code a bit more apparent. There are a few remaining that I'll take a look at in a bit

[github-actions]

📊 Metrics Diff Analysis Report

Summary

• New queries: 27
• Dropped queries: 21
• Plan changed + metrics changed: 16
• Plan unchanged + metrics changed: 0

ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

• New queries: Queries added in this PR
• Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
• Plan changed + metrics changed: The query plan has changed along with planner metrics.
• Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

New Queries

Count of new queries by file:

• yaml-tests/src/test/resources/join-tests.metrics.yaml: 26
• yaml-tests/src/test/resources/sql-functions.metrics.yaml: 1

Dropped Queries

The following queries with metrics were removed:

• yaml-tests/src/test/resources/join-tests.metrics.yaml:2: EXPLAIN select fname, lname from emp inner join project on emp_id = emp.id inner join dept on dept_id = dept.id and dept.name = 'Sales';
• yaml-tests/src/test/resources/join-tests.metrics.yaml:17: EXPLAIN select dept.name, project.name from emp, dept, project where emp.dept_id = dept.id and project.emp_id = emp.id;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:31: EXPLAIN select dept.name, project.name from emp inner join dept on emp.dept_id = dept.id inner join project on project.emp_id = emp.id;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:59: EXPLAIN select * from (select dept.name, project.name from emp, dept, project where emp.dept_id = dept.id and project.emp_id = emp.id) X;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:73: EXPLAIN select (*) from (select dept.name, project.name from emp, dept, project where emp.dept_id = dept.id and project.emp_id = emp.id) X;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:87: EXPLAIN select dept.name, project.name from project inner join emp on emp.id = project.emp_id inner join dept on emp.dept_id = dept.id;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:101: EXPLAIN select dept.id, dept.name, project.name from emp, dept, project where emp.dept_id = dept.id and project.emp_id = emp.id and dept.id not in (1)
• yaml-tests/src/test/resources/join-tests.metrics.yaml:117: EXPLAIN select dept.id, dept.name, project.name from emp inner join dept on emp.dept_id = dept.id and dept.id not in (1) inner join project on project.emp_id = emp.id
• yaml-tests/src/test/resources/join-tests.metrics.yaml:133: EXPLAIN select project.id, project.name from emp, dept, project where emp.dept_id = dept.id and project.emp_id = emp.id and dept.id not in (3) and project.id not in (3)
• yaml-tests/src/test/resources/join-tests.metrics.yaml:150: EXPLAIN select dept.id, dept.name, project.name from emp, dept, project where emp.dept_id = dept.id and project.emp_id = emp.id and dept.id in (1, 2)
• yaml-tests/src/test/resources/join-tests.metrics.yaml:166: EXPLAIN select * from ja join jb using(c1) join jd using(c1);
• yaml-tests/src/test/resources/join-tests.metrics.yaml:179: EXPLAIN select jua.c5, jub.c5, jud.c5 from jua join jub using(c1) join jud using(c1);
• yaml-tests/src/test/resources/join-tests.metrics.yaml:193: EXPLAIN select * from jua join (select * from jub join jud using(c1)) as X using (c1);
• yaml-tests/src/test/resources/join-tests.metrics.yaml:208: EXPLAIN select ja.c1, ja.c2, jb.c3 from ja inner join jb on true;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:220: EXPLAIN select ja.c1, ja.c2, jb.c3 from ja, jb where true;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:232: EXPLAIN select ja.c1, ja.c2, jb.c3 from ja inner join jb on false;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:244: EXPLAIN select ja.c1, ja.c2, jb.c3 from ja, jb where false;
• yaml-tests/src/test/resources/join-tests.metrics.yaml:256: EXPLAIN select ja.c1, ja.c2, jb.c3 from ja inner join jb on cast(null as boolean);
• yaml-tests/src/test/resources/join-tests.metrics.yaml:269: EXPLAIN select ja.c1, ja.c2, jb.c3 from ja, jb where cast(null as boolean);
• yaml-tests/src/test/resources/join-tests.metrics.yaml:281: EXPLAIN select ja.c1, ja.c2, jb.c3 from ja inner join jb on null;

This list has been truncated. There is 1 remaining dropped queries.

The reviewer should double check that these queries were removed intentionally to avoid a loss of coverage.

Plan and Metrics Changed

These queries experienced both plan and metrics changes. This generally indicates that there was some planner change
that means the planning for this query may be substantially different. Some amount of query plan metrics change is expected,
but the reviewer should still validate that these changes are not excessive.

Total: 16 queries

Statistical Summary (Plan and Metrics Changed)

task_count:

• Average change: +935.8
• Average regression: +935.8
• Median change: +1126
• Median regression: +1126
• Standard deviation: 927.8
• Standard deviation of regressions: 927.8
• Range: +20 to +3865
• Range of regressions: +20 to +3865
• Queries changed: 16
• Queries regressed: 16

transform_count:

• Average change: +184.4
• Average regression: +184.4
• Median change: +225
• Median regression: +225
• Standard deviation: 180.6
• Standard deviation of regressions: 180.6
• Range: +4 to +746
• Range of regressions: +4 to +746
• Queries changed: 16
• Queries regressed: 16

transform_yield_count:

• Average change: +67.6
• Average regression: +67.6
• Median change: +88
• Median regression: +88
• Standard deviation: 61.2
• Standard deviation of regressions: 61.2
• Range: +1 to +238
• Range of regressions: +1 to +238
• Queries changed: 16
• Queries regressed: 16

insert_new_count:

• Average change: +179.5
• Average regression: +179.5
• Median change: +209
• Median regression: +209
• Standard deviation: 195.1
• Standard deviation of regressions: 195.1
• Range: +3 to +826
• Range of regressions: +3 to +826
• Queries changed: 16
• Queries regressed: 16

insert_reused_count:

• Average change: +2.3
• Average regression: +2.3
• Median change: +2
• Median regression: +2
• Standard deviation: 2.0
• Standard deviation of regressions: 2.0
• Range: +1 to +8
• Range of regressions: +1 to +8
• Queries changed: 10
• Queries regressed: 10

Significant Regressions (Plan and Metrics Changed)

There was 1 outlier detected. Outlier queries have a significant regression in at least one field. Statistically, this represents either an increase of more than two standard deviations above the mean or a large absolute increase (e.g., 100).

• yaml-tests/src/test/resources/join-tests.metrics.yaml:525: EXPLAIN select dept.id, dept.name, project.name from emp, dept, project where emp.dept_id = dept.id and project.emp_id = emp.id and dept.id in (1, 2)
  • old explain: [IN arrayDistinct(promote(@c39 AS ARRAY(LONG)))] | INJOIN q0 -> { SCAN([IS DEPT, EQUALS q0]) } | FLATMAP q1 -> { SCAN([IS PROJECT]) | FLATMAP q2 -> { SCAN([IS EMP]) | FILTER _.DEPT_ID EQUALS q1.ID AND q2.EMP_ID EQUALS _.ID AS q3 RETURN q2 } AS q2 RETURN (q1.ID AS ID, q1.NAME AS _1, q2.NAME AS _2) }
  • new explain: [IN arrayDistinct(promote(@c39 AS ARRAY(LONG)))] | INJOIN q0 -> { SCAN([IS DEPT, EQUALS q0]) } | FLATMAP q1 -> { ISCAN(EMPDEPT [EQUALS q1.ID]) | FLATMAP q2 -> { SCAN([IS PROJECT]) | FILTER _.EMP_ID EQUALS q2.ID AS q3 RETURN q3 } AS q3 RETURN (q1.ID AS ID, q1.NAME AS _1, q3.NAME AS _2) }
  • task_count: 4963 -> 8828 (+3865)
  • transform_count: 1903 -> 2649 (+746)
  • transform_yield_count: 287 -> 525 (+238)
  • insert_new_count: 733 -> 1559 (+826)
  • insert_reused_count: 102 -> 110 (+8)

Minor Changes (Plan and Metrics Changed)

In addition, there were 15 queries with minor changes.

[alecgrieser]

I think I've responded to everything now, though for some of them, I've commented rather than make a code change

[alecgrieser]

It's possible... I'd rather we do this in a separate PR, though, as we're kind of far afield from the core bug fix at this point