Use Node as isDeleteTarget argument

[losipiuk]

Fixes: #6294
Replaces: #6288

As a followup we probably should pursue an approach to store Analysis.target as a NodeRef instead QualifiedObjectName. There is a chance that a single query contains a table that is a delete target more than once. We should not treat all those occurrences as delete target.

[sopel39]

make sure all tests pass (I think complex delete is tested also in raptor)

[losipiuk]

@martint We probably want this one for 348 still.

[losipiuk]

make sure all tests pass (I think complex delete is tested also in raptor)

Sure.

Any proposals for extra coverage to be added are welcome.

[findepi]

LGTM

This fixes two problems:

• VIEW that contains unqalified names inside

  • thanks for the test with Presto VIEW
  • please add tests with Hive VIEW as well, to rule out any other implications -- this bug is fixed, but it uncovered general gap in our testing of views

• DELERE FROM t that uses t in subquery as well

  • please add a test for that as well (i think there might have been no end-user visible effect of the bug ("innocent bug") though)

[kasiafi]

DELERE FROM t that uses t in subquery as well
please add a test for that as well (i think there might have been no end-user visible effect of the bug ("innocent bug") though)

I don't think this is addressed as long as Analysis performs the check by QualifiedObjectName and not by NodeRef. That requires recording target by NodeRef, as @losipiuk suggested.

i think there might have been no end-user visible effect of the bug

The only effect so far of tagging a TableScanNode incorrectly as the delete target is that a certain optimization is disabled in that branch of plan (namely, the rewrite of filtering semi-join to inner). However, the flag forDelete was meant to be used to disable other optimizations that are not relevant in the context of delete (e.g. join flipping).

I think that not taking this flag into consideration is a more serious issue than setting it where it doesn't belong.

[findepi]

I think that not taking this flag into consideration is a more serious issue than setting it where it doesn't belong.

Today. But this could change in the future.

Would it be possible to have a validator that verifies the flag is properly set and retained during optimizations?
We could also verify it is not set for ineligible table scans.
(of course, the second check is only possible after the Analysis changes as proposed)

(i don't mean the validator should be part of this PR)

[martint]

We should fix this, not just leave a TODO. Otherwise, any query such as DELETE FROM t WHERE t.v IN (SELECT ... FROM t WHERE ...) will do the wrong thing. That's actually a regression from how this used to work before isDeleteTarget was added.

[findepi]

@martint yes, we discussed that.
We want to fix the user-visible bug for views first (this PR)
and i then the (currently user-invisible) bug will be follow up

[sopel39]

That's actually a regression from how this used to work before isDeleteTarget was added.

Currently this flag prevents semi_join->join rewrite, so when it's true for target table scans, previous behavior is preserved.

[sopel39]

@losipiuk you mixed up commits from other PR

[losipiuk]

@losipiuk you mixed up commits from other PR

thanks :)