Don't ignore failed optimizer rules

[crepererum]

Describe the bug
Some logical optimizer rules like type coercion are actually essential for correct query execution, but are currently silently ignored when they failed.

To Reproduce
TBD

Expected behavior
Don't skip failed rules (this probably requires us to remove the non-failing OptimizerRule::optimize method).

Workaround
This behavior is controlled by a config option (that defaults to true meaning to silently skip failed errors).

You can see this in the datafusion cli:

DataFusion CLI v20.0.0
❯ show all;
+-----------------------------------------------------------+------------+
| name                                                      | setting    |
+-----------------------------------------------------------+------------+
...
| datafusion.optimizer.skip_failed_rules                    | true       |
...
+-----------------------------------------------------------+------------+
35 rows in set. Query took 0.038 seconds.

Datafusion can be configured to fail hard if an optimizer rule fails like

❯ -- default behavior
❯ set datafusion.optimizer.skip_failed_rules = false;
0 rows in set. Query took 0.023 seconds.

Additional context
See this discussion.

[jackwener]

Make sense to me. It's a good discussion point.
Some rule is essential, we shouldn't tolerate them failed.
BTW, I think we also should make optimizer rule more stable instead of ignoring error.
IMHO, I think it's not good that tolerate error in optimizer. If a rule want to tolerate failed, I think it may be better handle inside itself.

[alamb]

I think the original "ignore optimizer rule if error" was added (maybe by @andygrove or @avantgardnerio ) because at that time there was no way to distinguish between "Optimizer didn't error but could not handle the query" and "optimizer hit a real error"

Now that Optimizer::try_optimize exists, which can distinguish between those two cases, I think removing OptimizerRile::optimize is 👍

[alamb]

Related #4619

[HaoYang670]

Some logical optimizer rules like type coercion are actually essential for correct query execution

Some rule is essential, we shouldn't tolerate them failed

at that time there was no way to distinguish between "Optimizer didn't error but could not handle the query" and "optimizer hit a real error"

The type coercion and other rules for correctness should be moved in front of the optimizer.
It is the duty of the optimizer to make the query plan to achieve better performance, but not to correct the query.
In another word, the Datafusion should work even if we remove the optimizer.

It could be good if we could extract the type coercion from the optimizer and put it into a type checker for correcting the query. The pipeline could be very similar to a compiler:

SQL ---(parser)---> logical plan ---(type checker)---> verified logical plan ---(logical optimizer)---> optimized logical plan ...
          |                                 |
         \|/                               \|/
       Syntax error                     Type error

If we could do this, one different idea of the return type of the optimizer is that I think the optimizer should return an Option<LogicalPlan> instead of Result<Option<LogicalPlan>> or Result<LogicalPlan>. Because we should not allow an error (even if it is an Internal Error) returned from the optimizer, with the assumption that the input plan has been verified. The variant of the returned values should only be:

1. None: when the plan cannot be optimized.
2. Some(plan): return the optimized plan
3. panic!(..): there is a bug in the optimizer.

[crepererum]

On application of failing during an optimization may also be to detect malformed plans early. E.g. while working on #4370 I've figured that we could reject malformed regex patterns that are available during planning as literal early. I'm wondering if this should then be done by failing the optimization (and by fixing this very issue) or by inserting "terminal"/"always error" expressions into the plan that are later picked up by the physical planner.

[alamb]

The type coercion and other rules for correctness should be moved in front of the optimizer.
It is the duty of the optimizer to make the query plan to achieve better performance, but not to correct the query.
In another word, the Datafusion should work even if we remove the optimizer.

I agree this would be a good change

@liukun4515 made a similar observation / suggestion #3582

I'm wondering if this should then be done by failing the optimization (and by fixing this very issue) or by inserting "terminal"/"always error" expressions into the plan that are later picked up by the physical planner.

I recommend the inserting an "always error" expr node. The rationale being that you could have branches that are never actually hit in the query

A trivial example is like

CASE
  WHEN FALSE THEN 1 / 0 
  WHEN TRUE THEN 5.0
END

[alamb]

panic!(..): there is a bug in the optimizer.

I would personally rather have an InternalError rather than a panic

[andygrove]

original "ignore optimizer rule if error" was added (maybe by @andygrove or @avantgardnerio ) because at that time there was no way to distinguish between "Optimizer didn't error but could not handle the query" and "optimizer hit a real error"

That's correct. Now that we have the try_optimize method I support failing on errors and we will soon fix cases where rules incorrectly return errors.

[HaoYang670]

I'm wondering if this should then be done by failing the optimization (and by fixing this very issue) or by inserting "terminal"/"always error" expressions into the plan that are later picked up by the physical planner.

My suggestion is to fail as early as possible if we have detected some errors in the logical plan.

I recommend the inserting an "always error" expr node. The rationale being that you could have branches that are never actually hit in the query

A trivial example is like

CASE
  WHEN FALSE THEN 1 / 0 
  WHEN TRUE THEN 5.0
END

Why do we cover up the error, instead of exposing them? Besides, we should not return a compile error at runtime, even if the branch won't be hit. No static typed language would do this (I guess SQL is not a dynamic typed language 🤔).

[HaoYang670]

I tried to remove the config skip_failed_rules and met several test failures tracked in #4685.
Feel free to update the task-list if you find more and welcome to fix them if you have time!

[alamb]

Why do we cover up the error, instead of exposing them? Besides, we should not return a compile error at runtime, even if the branch won't be hit. No static typed language would do this (I guess SQL is not a dynamic typed language 🤔).

Well I think the rationale / question is if SQL expression semantics allow / require eager evaluation or if delayed evaluation

A more realistic example might be to use the CASE to explicitly guard against a divide by zero

CASE
  WHEN num <> 0 THEN value / num
  ELSE  0.0
END

So in that case evaluating value/num might cause a divide by zero if it were done prior to evaluating the WHEN guard 🤔

[HaoYang670]

Well I think the rationale / question is if SQL expression semantics allow / require eager evaluation or if delayed evaluation

These are the variants for runtime behavior, not related to compile time checking I think.
I've done some tests on Postgres, and find the expression when FALSE then ... will be removed before type checking, so if you have the expr when FALSE then 1/0 in your query, it is always valid: https://onecompiler.com/postgresql/3yskvh35x

But if the invalid expression can not be removed at compile time, a Divide by zero error will always return before executing the query, even if the invalid code won't be touched at runtime: https://onecompiler.com/postgresql/3yskvqaxf