Skip to content

Move validation for InsertInto nodes into the analyzer's validation phase#2283

Closed
fulghum wants to merge 2 commits intomainfrom
fulghum/dolt-7313
Closed

Move validation for InsertInto nodes into the analyzer's validation phase#2283
fulghum wants to merge 2 commits intomainfrom
fulghum/dolt-7313

Conversation

@fulghum
Copy link
Contributor

@fulghum fulghum commented Jan 22, 2024

The resolveInsertRows analyzer rule contains various validation checks for InsertInto plan nodes. A customer reported a panic in dolthub/dolt#7313 because an InsertInto validation error wasn't caught before the assignExecIndexes rule ran. Moving the validation from resolveInsertRows up into a new rule in the validation batch catches the validation error before the assignExecIndexes rule runs and prevents the panic the customer hit.

The only difference I see in this refactoring is that the new validateInserts rule doesn't invoke the analyzer on InsertInto.Source when the source is a TriggerExecutor. This is still done in the resolveInsertRows rule and the result is used to transform the node being processed, but the functions used by the new validation rule (i.e existsNonZeroValueCount, validateColumns, validateValueCounts) don't seem to actually need the TriggerExecutor to be analyzed independently yet at that point.

Related to: dolthub/dolt#7313

@max-hoffman
Copy link
Contributor

closing in favor of #2286

@fulghum fulghum deleted the fulghum/dolt-7313 branch January 23, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants