Support incremental_strategy parameter and new insert_overwrite strategy#2195
Support incremental_strategy parameter and new insert_overwrite strategy#2195SuchodolskiEdvin wants to merge 1 commit into
Conversation
SuchodolskiEdvin
commented
Jun 2, 2026
SuchodolskiEdvin
commented
- updated proto with new parameters
- added new tests
- added validation for chosen incremental_strategies
- added new insert_overwrite strategy logic
SuchodolskiEdvin
force-pushed
the
insert_overwrite_incremental_strategy
branch
from
9f72a8c to
9724071
Compare
SuchodolskiEdvin
force-pushed
the
insert_overwrite_incremental_strategy
branch
from
9724071 to
41d152d
Compare
| const backtickedColumns = columns.map(column => `\`${column}\``); | ||
| const resolveTargetTable = this.resolveTarget(target); | ||
|
|
||
| return `CREATE OR REPLACE TEMP TABLE \`${stagingTableUnqualified}\` AS ( |
There was a problem hiding this comment.
can you split separate SQL statements into granular Task.statement calls?
There was a problem hiding this comment.
I believe we can't split this SQL because we are using TEMP table. We could in case we would use persistent table.
| MERGE ${resolveTargetTable} T | ||
| USING \`${stagingTableUnqualified}\` S | ||
| ON FALSE | ||
| WHEN NOT MATCHED BY SOURCE AND ${partitionBy} IN UNNEST(partitions_for_replacement) ${updatePartitionFilter ? `and T.${updatePartitionFilter}` : ""} THEN |
There was a problem hiding this comment.
Such code T.${updatePartitionFilter} will only work when updatePartitionFilter has exactly one expression?
There was a problem hiding this comment.
Yes, you are correct. It is a known limitation that T.${updatePartitionFilter} only works for simple expressions (and fails on multi-expression SQL). Current implementation is designed to match the existing behavior of the standard MERGE strategy to maintain consistency between the two strategies for now. The fix of using updatePartitionFilter with several expression will be introduced in a separate PR.
| return this.mergeInto( | ||
| table.target, | ||
| columns, | ||
| incrementalQuery, |
There was a problem hiding this comment.
Can you deduplicate this with a call below?
| switch (this.proto.incrementalStrategy) { | ||
| case dataform.IncrementalStrategy.INSERT_OVERWRITE: | ||
| if (!this.proto.bigquery || !this.proto.bigquery.partitionBy) { | ||
| this.session.compileError( | ||
| new Error("incrementalStrategy 'insert_overwrite' requires 'partitionBy' to be set."), | ||
| config.filename, | ||
| this.proto.target | ||
| ); | ||
| } | ||
| break; | ||
| case dataform.IncrementalStrategy.MERGE: | ||
| if (!this.proto.uniqueKey || this.proto.uniqueKey.length === 0) { | ||
| this.session.compileError( | ||
| new Error("incrementalStrategy 'merge' requires 'uniqueKey' to be set."), | ||
| config.filename, | ||
| this.proto.target | ||
| ); | ||
| } | ||
| break; | ||
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
let's move it into sub-function for readability
| expect(result.compile.compiledGraph.tables[0].incrementalStrategy).equals( | ||
| dataform.IncrementalStrategy.INSERT_OVERWRITE | ||
| ); |
There was a problem hiding this comment.
let's check the whole object here (same below)
| string reservation = 27; | ||
|
|
||
| // Optional. The incremental strategy to use when updating the table. | ||
| // Defaults to MERGE if uniqueKey is configured, or APPEND otherwise. |
There was a problem hiding this comment.
I wouldn't include it into config protos (because it's run-time strategy and not related to compiled DAG generation)
SuchodolskiEdvin
force-pushed
the
insert_overwrite_incremental_strategy
branch
3 times, most recently
from
277c48b to
f11f8f3
Compare
- updated proto with new parameters - added new tests - added validation for chosen incremental_strategies - added new insert_overwrite strategy logic
SuchodolskiEdvin
force-pushed
the
insert_overwrite_incremental_strategy
branch
from
f11f8f3 to
1942a72
Compare
2 participants
