-
Notifications
You must be signed in to change notification settings - Fork 667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: EXPOSED-334 Support MERGE statement #2047
Conversation
3f347dd
to
489a7e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy with the state of the source API.
Left some general comments, but mostly about testing.
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Queries.kt
Outdated
Show resolved
Hide resolved
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/Statement.kt
Outdated
Show resolved
Hide resolved
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/MergeBaseStatement.kt
Outdated
Show resolved
Hide resolved
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/MergeBaseStatement.kt
Outdated
Show resolved
Hide resolved
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/vendors/FunctionProvider.kt
Outdated
Show resolved
Hide resolved
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/MergeBaseTest.kt
Outdated
Show resolved
Hide resolved
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/MergeSelectTest.kt
Outdated
Show resolved
Hide resolved
object SourceNoAutoId : IdTable<Int>("test_source_no_auto_id") { | ||
override val id = integer("id").entityId() | ||
val value = varchar("value", 128) | ||
override val primaryKey = PrimaryKey(id) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for testing with an IdTable
like this and not just a regular Table
with a primary key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was on purpose, I wanted to have table without id auto generation, to put the values I want and check that they are presented after merge (to test that ON condition is generated correctly)
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/MergeTableTest.kt
Outdated
Show resolved
Hide resolved
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/MergeTableTest.kt
Outdated
Show resolved
Hide resolved
… fix throwing exceptions
1cdad0c
to
5aa3660
Compare
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/vendors/SQLServerDialect.kt
Outdated
Show resolved
Hide resolved
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/MergeSelectTest.kt
Outdated
Show resolved
Hide resolved
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/MergeBaseTest.kt
Outdated
Show resolved
Hide resolved
…geTestTablesAndDefaultData() for tests
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/MergeBaseStatement.kt
Outdated
Show resolved
Hide resolved
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/vendors/FunctionProvider.kt
Show resolved
Hide resolved
dest: Table, | ||
source: Table, | ||
transaction: Transaction, | ||
whenBranches: List<MergeBaseStatement.WhenBranchData>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like MergeBaseStatement.WhenBranchData
is not readable as a standalone. It should always be used together and meaningful when reading.
What do you think about MergeStatement.WithWhen
or just MergeStatement.When
?
@bog-walk, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite often referenced as clause
in the documentations. So another options might be Clause
or WhenClause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e5l @obabichevjb MergeStatement.When
and MergeStatement.WhenClause
to me seem the most descriptive and to the point when used alone.
If we also want to keep it inline with the MergeWhenAction
enum passed to it, for example, then simply MergeStatement.When
would work well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated MR, removed any usages of the term "branch" and replaced it with the term "clause".
After 2 iterations I come up with the variant of usage only term "clause(s)" instead of "when"/"whenClauses". It may look more descriptive and simpler in the context of merge command, because there is only one term. With that variant interface of FunctionProvider.merge()
looks like:
open fun merge(
dest: Table,
source: Table,
transaction: Transaction,
clauses: List<MergeStatement.Clause>,
on: Op<Boolean>?
)
And the Clause data class:
data class Clause(
val action: ClauseAction,
val arguments: List<Pair<Column<*>, Any?>>,
val and: Op<Boolean>?,
/** ... */
val deleteWhere: Op<Boolean>? = null
)
enum class ClauseAction {
INSERT, UPDATE, DELETE
}
Minor but also action
parameter of Clause
class moved to the first position, as it defines the meaning of the following arguments.
What do you think about such a variant?
One commit before there is an option with MergeStatement.When
data class, but I found out that it looks quite weird for the list parameters to call them whens
or whenClauses
…vider::mergeSelect() functions; rename MergeBaseStatement to MergeStatement
…ntext of merge command. Rename `MergeBaseStatement.WhenBranchData` to `MergeBaseStatement.When`, and `MergeBaseStatement.MergeWhenAction` to `MergeBaseStatement.WhenAction`
Please update the PR description |
Updated |
This PR introduces a new
mergeFrom
method query method, allowing execute underlying sql merge command. The merge method supports merging data from one table or select query into another table, with conditional logic for matched and unmatched rows.The method can be used in the following way:
Merge command is supported by the databases: SQL Server, Postgres, Oracle, H2