Feat!: Add support for merge_filter and dbt incremental_predicates for Incremental By Unique Key#3540
Conversation
| name db.employees, | ||
| kind INCREMENTAL_BY_UNIQUE_KEY ( | ||
| unique_key name, | ||
| incremental_predicates source.salary > 0 |
There was a problem hiding this comment.
Does this support @start_* and @end_* macros? Can we add a test for this?
There was a problem hiding this comment.
We need to make sure they are resolved at evaluation time and not load time
There was a problem hiding this comment.
Yes they are supported, but I'm not sure if they are resolved at evaluation time. I'll verify and adjust if they're not
themisvaltinos
force-pushed
the
themis/incremental_predicates
branch
from
66ec744 to
79bd81c
Compare
themisvaltinos
changed the title
| value = self._parse_wrapped( | ||
| lambda: _parse_macro_or_clause(self, self._parse_conjunction), optional=True | ||
| ) |
There was a problem hiding this comment.
Do we need the parentheses here?
There was a problem hiding this comment.
no we don't I just left them as an optional in case a user had it inside parenthesis but can remove them
There was a problem hiding this comment.
The parentheses can be parsed without this, because _parse_conjunction can reach _parse_primary which knows how to parse parenthesized expressions. So I don't see a reason to treat this as special syntax.
There was a problem hiding this comment.
Oh right then removing this
| if isinstance(v, str): | ||
| v = v.strip() | ||
| if v.startswith("("): | ||
| v = v[1:-1] | ||
| return d.parse_one(v, into=exp.Expression, dialect=get_dialect(values)) |
There was a problem hiding this comment.
So in the case of when_matched we needed the parentheses to fix some ambiguity issues in the parser. In this case since I believe we can omit them, so doing this here won't be necessary.
Also, I'm not sure if into=exp.Expression is needed here, looks redundant?
| lambda: _parse_macro_or_clause(self, self._parse_when_matched), | ||
| optional=True, | ||
| ) | ||
| elif name == "merge_filters": |
There was a problem hiding this comment.
[Nit] I wonder if this should be singular, i.e. merge_filter. I'm personally fine either way, just wanted to see what others think. Perhaps the plural form creates an expectation that a list or tuple is valid (even though it's not)?
There was a problem hiding this comment.
yes I'm fine with either, the plural wa if there were many conditions in the conjunction, but merge_filter works too
themisvaltinos
force-pushed
the
themis/incremental_predicates
branch
from
79bd81c to
b0d233f
Compare
themisvaltinos
force-pushed
the
themis/incremental_predicates
branch
from
b0d233f to
3c640e9
Compare
themisvaltinos
changed the title
3 participants
This update introduces
merge_filtersinINCREMENTAL_BY_UNIQUE_KEYkind models and also adds support for dbt projects withincremental_predicates. It updates the incremental logic for engines that support theMERGEstrategy, by incorporating ability to filter data during the merge operation.When using this feature, both source and target aliases can be used to distinguish between the source and target columns similar to
WHEN MATCHED.Example Model
MODEL ( name db.test, kind INCREMENTAL_BY_UNIQUE_KEY ( unique_key [purchase_order_id], merge_filters ( source._operation <> 1 AND target.start_date > DATEADD(day, -7, CURRENT_DATE) ) ) );The above predicates are then used into SQLMesh’s
MERGEstatements:This support extends to an existing dbt project to be converted to the equivalent sqlmesh INCREMENTAL_BY_UNIQUE_KEY model, as it aligns with dbt’s incremental predicates.