Incremental predicates in Merge may affect key predicates

[mi-volodin]

Describe the bug

To be honest, I am not sure if that is a bug or a feature. But I am raising this issue to discuss it.

The "problem" exists here when we produce the merge predicates.

The summary of the problem is that when incremental_predicates are used they are inserted into the predicates block explicitly. That might lead to some interesting "SQL injections" that will ignore the merge_keys condition completely.

E.g., just to illustrate the injection

incremental_predicates = 'True OR False'
merge_keys = 'PK'

will result in the merge condition that will be evaluated to True:

True OR False AND target.PK <=> source.PK

I personally think that this should not be allowed and incremental predicates should be always injected with (), so that:

(True OR False) AND target.PK <=> source.PK

See example for more business related case.

Steps To Reproduce

The illustrative example is as follows.
Let's assume we have an event table T with a single key attribute ID and some event_ts timestamp attribute. And we are doing the following merge statement

merge into T as tgt
using (
    ...
) as src
on
    {{ incremental_predicates }}
    AND tgt.ID <=> src.ID

In certain use cases when pruning by PK doesn't work well (e.g. UUID as string for Delta storage) we might want to instruct the Databricks to reduce the amount of information to fetch from S3 before performing merge.

Let's assume we know ts1 and ts2 that are min/max(event_ts) from source. Without the loss of generality I'd assume they are already instantiated as Databricks variables (i.e. I can use them directly in SQL).

Then I can use incremental_predicates = tgt.event_ts between ts1 and ts2. The compiled code will really boost the merge execution.

However, sometimes the team combines sources to be ingested into the table. For example, when some "backfilling" happen, and there's a dense batch of events that has a different span of event_ts. The described behavior was discovered exactly for this case, when we add another interval with or condition

incremental_predicates = """
    tgt.event_ts between ts1 and ts2
    or tgt.event_ts between ts3 and ts4
"""

the compiled merge statement (as you might observe) would ignore the merge keys

merge into T as tgt
using (
    ...
) as src
on
    tgt.event_ts between ts1 and ts2
    or tgt.event_ts between ts3 and ts4
    AND tgt.ID <=> src.ID

Expected behavior

At least for the engineering team that stepped into this that was absolutely counterintuitive that they MUST use () to isolate incremental_predicate manually.

Thus, the proposal is to force the wrap-up into () for the case the incremental_predicates are added.

[mi-volodin]

I can propose a fix as PR, but before investing into it I'd like to understand if it is also seen as a bug.

The thing is - someone might actually exploit this injections... So I'd change the behaviour, but if we have concerns then maybe would add a feature flag to turn off the parenthesis wrap-up.

[github-actions]

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please remove the stale label or comment on the issue.

[mi-volodin]

The PR is still waiting some guiding review from the maintainer. Issue is still valid and has not been resolved yet.

[mi-volodin]

@sd-db I'd appreciate your feedback on the issue. It can be easily fixed and I can prepare the PR, but it requires design decision. For convenience - citing the example that illustrates the problem from the description.

---

E.g., just to illustrate the injection

incremental_predicates = 'True OR False'
merge_keys = 'PK'

will result in the merge condition that will be evaluated to True:

True OR False AND target.PK <=> source.PK

I personally think that this should not be allowed and incremental predicates should be always injected with (), so that:

(True OR False) AND target.PK <=> source.PK