Support REPLACE INTO for INSERT statements

[fmeringdal]

Which issue does this PR close?

Closes #12515 .

Are these changes tested?

Yes, in datafusion/core/tests/user_defined/insert_operation.rs.

Are there any user-facing changes?

Yes, overwrite boolean flags has been replaced by InsertOp enum in the following APIs and configs: TableProvider::insert_into, FileSinkConfig, DataFrameWriteOptions.

[alamb]

Thank you for this contirbution @fmeringdal -- this code looks good to me. I am worried about the lack of tests, however

Basically if there isn't test coverage we could potentially break this feature without realizing it in some future refactor.

maybe we could add something in

https://github.com/apache/datafusion/tree/main/datafusion/core/tests/user_defined

That shows that a mock up user defined TableProvider will have insert_into called with the correct value of replace_into when the relevant SQL is planned 🤔

It is unfortunate we don't seem to already have such a test

[alamb]

Can you please document this new parameter and explain what it means (bonus points for documenting what _overwrite means too)

[fmeringdal]

That shows that a mock up user defined TableProvider will have insert_into called with the correct value of replace_into when the relevant SQL is planned

Test added in datafusion/core/tests/user_defined/insert_operation.rs.

Can you please document this new parameter and explain what it means (bonus points for documenting what _overwrite means too)

Documentation added to the variants of the InsertOp enum.

[fmeringdal]

Thanks for the quick review @alamb 🙏 I will get some tests added.

I wanted to run an idea by you before I implement the change to restructure the WriteOp enum from:

pub enum WriteOp {
    InsertOverwrite,
    InsertReplace,
    InsertInto,
    Delete,
    Update,
    Ctas,
}

To:

pub enum WriteOp {
    Insert(InsertOp),
    Delete,
    Update,
    Ctas,
}

pub enum InsertOp {
    Append,    // Represents a regular INSERT INTO operation
    Overwrite, // Represents an INSERT OVERWRITE operation
    Replace,   // Represents an INSERT OR REPLACE operation
}

The purpose of such a change would be so that TableProvider::insert_into could take an op: InsertOp argument instead of two boolean arguments for overwrite and replace_into . I think it makes it slightly more convenient for implementors of the insert_into method to match on an enum rather than checking two booleans.
What do you think?

[alamb]

The purpose of such a change would be so that TableProvider::insert_into could take an op: InsertOp argument instead of two boolean arguments for overwrite and replace_into . I think it makes it slightly more convenient for implementors of the insert_into method to match on an enum rather than checking two booleans.
What do you think?

I agree it would be a good change 👌

Thank you!

[alamb]

Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look

[fmeringdal]

Ready for another round of review! Changes applied since the first iteration:

• InsertOp enum has been introduced and replaced the overwrite bool. This introduced a few breaking changes as described in the PR description. Any other places I need to document these breaking changes?
• Test added to datafusion/core/tests/user_defined/insert_operation.rs.
• Handle both REPLACE INTO and INSERT OR REPLACE INTO

[alamb]

Thank you @fmeringdal for this PR and for making it easy to review the PR (nicely documented, nicely written code, and nicely tested) 🏆

I had this PR checked out anyways so I took the liberty of merging up from main to resolve the conflicts, as well as added a license notice to the file and fixed a clippy issue.

[alamb]

❤️

[alamb]

Merged up to get CI fix / hopefully pass

[alamb]

🚀