Skip to content
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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

obabichevjb
Copy link
Collaborator

No description provided.

@bog-walk bog-walk self-assigned this Apr 16, 2024
@obabichevjb obabichevjb changed the title First simplified version of the merge command implementation. feat: merge command implementation Apr 17, 2024
@obabichevjb obabichevjb changed the title feat: merge command implementation feat: EXPOSED-334 Support MERGE statement Apr 17, 2024
@obabichevjb obabichevjb force-pushed the obabichev/merge-statement branch 2 times, most recently from 3f347dd to 489a7e1 Compare April 29, 2024 09:31
Copy link
Member

@bog-walk bog-walk left a 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.

Comment on lines 413 to 417
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)
}
Copy link
Member

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?

Copy link
Collaborator Author

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)

@obabichevjb obabichevjb requested a review from bog-walk May 13, 2024 08:50
dest: Table,
source: Table,
transaction: Transaction,
whenBranches: List<MergeBaseStatement.WhenBranchData>,
Copy link
Member

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?

Copy link
Collaborator Author

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

Copy link
Member

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.

Copy link
Collaborator Author

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`
@obabichevjb obabichevjb requested review from bog-walk and e5l May 22, 2024 09:35
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.

None yet

3 participants