Skip to content

Conversation

@alex-spies
Copy link
Contributor

In the case that only some of the fields are missing, this shouldn't be much better than just inserting field extractions that insert the nulls, which should already happen.

This may affect performance negatively in cases when all the fields required to run a plan are locally missing - we should check that.

In the case that only some of the fields are missing, this shouldn't be
much better than just inserting field extractions that insert the nulls,
which should already happen.

This may affect performance negatively in cases when all the fields
required to run a plan are locally missing - we should check that.
@idegtiarenko
Copy link
Contributor

idegtiarenko commented May 8, 2025

I tried to measure this change locally, this are my results:

Before
| 99th percentile latency |               from_idx_limit_1 |   66.9602      |     ms |
| 99th percentile latency |              from_idx_limit_10 |   71.1003      |     ms |
| 99th percentile latency |            from_idx_limit_1000 |  100.996       |     ms |
| 99th percentile latency |           from_idx_limit_10000 |  171.693       |     ms |
| 99th percentile latency |     from_idx_limit_1_drop_null |   47.392       |     ms |
| 99th percentile latency |    from_idx_limit_10_drop_null |   48.0725      |     ms |
| 99th percentile latency |  from_idx_limit_1000_drop_null |   59.2542      |     ms |
| 99th percentile latency | from_idx_limit_10000_drop_null |  149.28        |     ms |

After
| 99th percentile latency |               from_idx_limit_1 |   40.9169      |     ms |
| 99th percentile latency |              from_idx_limit_10 |   41.8376      |     ms |
| 99th percentile latency |            from_idx_limit_1000 |  104.694       |     ms |
| 99th percentile latency |           from_idx_limit_10000 |  173.288       |     ms |
| 99th percentile latency |     from_idx_limit_1_drop_null |   37.0698      |     ms |
| 99th percentile latency |    from_idx_limit_10_drop_null |   36.5884      |     ms |
| 99th percentile latency |  from_idx_limit_1000_drop_null |   46.2376      |     ms |
| 99th percentile latency | from_idx_limit_10000_drop_null |  145.939       |     ms |

So with small results sets it does become faster and big results (even with lots of nulls) are not negatively affected.
Looks like worth pursuing?

@alex-spies
Copy link
Contributor Author

alex-spies commented May 8, 2025

I agree, the performance increase from not trying to optimize is not negligible in cases when this optimizer rule has to walk many fields.

I agree that this is worth pursuing. I'd like to be paranoid and double check which other optimizer rules may profit from explicit EVAL some_missing_field = NULL plan nodes during local logical optimization.

One rule that, I believe, sometimes benefits from the explicit EVAL some_missing_field = NULL injections is PropagateEvalFoldables. This one picks up explicit aliases, like EVAL some_missing_field = NULL, respects downstream renamings via additional aliases, and then inserts literal nulls into filters and evals downstream.

Filters and evals per se would remain covered - except in cases when there are aliasing steps (in projections) that obscure that a renamed attribute really points to a missing field, which could be replaced by a null.

ReplaceMissingFieldsWithNull could be amended to work like PropagateEvalFoldables does, and both could be refactored to share code; this would close out this gap; if we even care about it - as aliasing in the middle of the plan is rare and happens only when we can't combine projections due to e.g. MV_EXPAND being in the way. (LOOKUP JOIN, too, but this one is being worked on to be pushed down past projections)

@alex-spies
Copy link
Contributor Author

Note: I think this PR will make the changes to CombineProjections by @bpintea in #127264 obsolete because missing fields won't result in evals anymore that could even be propagated. Accordingly, if we go forward with this, I think we should simplify CombineProjections again by undoing that change, as it will be (confusing) dead code then.

@alex-spies
Copy link
Contributor Author

Interactions to check before moving forward with this:

  • PropagateEvalFoldables: should not really matter because ReplaceMissingFieldsWithNull should still update any Evals and Filters directly, but @bpintea may have an example where ReplaceMissingFieldsWithNull + PropagateEvalFoldables work together, because that's why his recent change to CombineProjections was necessary going by his comment.
  • PropagateEmptyRelation (maybe?) or whatever happens when all of the fields are locally missing.
  • PruneLeftJoinOnNullMatchingField

@alex-spies
Copy link
Contributor Author

Discussed with @idegtiarenko offline, the necessary work for this has grown due to LOOKUP JOIN:

  • PruneLeftJoinOnNullMatchingField depends on null literals added to the plan via Evals in case of missing fields.
  • PruneLeftJoinOnNullMatchingField can and likely should be reworked once LOOKUP JOIN takes expressions, e.g. LOOKUP JOIN idx ON left.field == right.otherfield, in which case we can just place/propagate null literals into the join itself, rather than walking the plan and looking for null literals.

@alex-spies
Copy link
Contributor Author

Closing for now, we can come back to this once we remodel joins to use expressions (like LOOKUP JOIN lu_idx ON left.field == right.otherfield) which will allow us to implement PruneLeftJoinOnNullMatchingField in another way.

@alex-spies alex-spies closed this Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants