Skip to content

Add dolt_preview_merge_conflicts table function#9270

Merged
tbantle22 merged 16 commits intomainfrom
taylor/conflicts-preview-table
Jun 10, 2025
Merged

Add dolt_preview_merge_conflicts table function#9270
tbantle22 merged 16 commits intomainfrom
taylor/conflicts-preview-table

Conversation

@tbantle22
Copy link
Contributor

@tbantle22 tbantle22 commented May 29, 2025

This table function shows which rows are conflicting for a table between two branches. Will error if there are schema conflicts

@tbantle22 tbantle22 changed the title Preview merge conflicts for table before attempting merge Add dolt_preview_merge_conflicts table function May 29, 2025
Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quick comments on the implementation. I think I got enough of an overview to discuss further iteration...

@tbantle22 tbantle22 force-pushed the taylor/conflicts-preview-table branch from 874c15a to 7507de1 Compare May 29, 2025 23:49
@tbantle22 tbantle22 marked this pull request as ready for review June 3, 2025 21:21
@tbantle22 tbantle22 force-pushed the taylor/conflicts-preview-table branch from c0ee319 to eec403b Compare June 5, 2025 22:30
@tbantle22 tbantle22 requested a review from Copilot June 5, 2025 23:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for the new table function dolt_preview_merge_conflicts along with extensive tests and refactors conflict table schema handling. Key changes include:

  • New test cases for dolt_preview_merge_conflicts and its error conditions in multiple query test files.
  • Renaming and refactoring of functions (e.g. CalculateConflictSchema) with updated conflict row handling using new descriptors and offsets.
  • Minor adjustments and improvements in downstream merge logic and functionality.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
go/libraries/doltcore/sqle/enginetest/dolt_queries_schema_merge.go Added test cases for preview merge conflicts with expected error strings.
go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go Expanded test coverage for various preview merge conflicts scenarios.
go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go Refactored conflict schema calculation and row iteration using new value descriptors and offsets.
go/libraries/doltcore/sqle/dtablefunctions/init.go Registered the new PreviewMergeConflictsTableFunction.
go/libraries/doltcore/sqle/dtablefunctions/dolt_preview_merge_conflicts_summary.go Updated branch-to-root resolution to use a new rootInfo struct.
go/libraries/doltcore/dtablefunctions/dolt_diff.go Corrected error message formatting.
go/libraries/doltcore/merge/* Added helper method and streamlined merge logic with new getters.
Comments suppressed due to low confidence (3)

go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go:56

  • The function name has been updated to 'CalculateConflictSchema' (exported), so please verify that all callers across the codebase have been updated to reflect this naming change.
confSch, versionMappings, err := CalculateConflictSchema(baseSch, ourSch, theirSch)

go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go:256

  • [nitpick] Consider adding inline comments or expanding the function's documentation to explain how the various offset values are computed and used when populating conflict rows. This would improve readability and ease future maintenance.
func PutConflictRowVals(ctx *sql.Context, confVal ConflictVal, row sql.Row, offsets ConflictOffsets, vds ConflictValueDescriptors, ns tree.NodeStore) error {

go/libraries/doltcore/sqle/dtablefunctions/dolt_preview_merge_conflicts_summary.go:299

  • [nitpick] Now that the function returns a 'rootInfo' struct with additional fields (rightCm and ancCm), it would be beneficial to enhance the documentation for this function, explaining the purpose of each field and the expected usage downstream.
func resolveBranchesToRoots(ctx *sql.Context, db dsess.SqlDatabase, leftBranch, rightBranch string) (rootInfo, error) {

Copy link
Contributor

@nicktobey nicktobey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright this looks good. I just have a couple points of confusion that I think can be cleared up with comments.

Base automatically changed from taylor/conflicts-preview to main June 9, 2025 21:53
@coffeegoddd
Copy link
Contributor

@tbantle22 DOLT

comparing_percentages
100.000000 to 100.000000
version result total
bbfbb8f ok 5937457
version total_tests
bbfbb8f 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
f1f9dec ok 5937457
version total_tests
f1f9dec 5937457
correctness_percentage
100.0

@tbantle22 tbantle22 merged commit 93301be into main Jun 10, 2025
21 of 22 checks passed
@tbantle22 tbantle22 deleted the taylor/conflicts-preview-table branch June 10, 2025 17:31
@coffeegoddd
Copy link
Contributor

@tbantle22 DOLT

comparing_percentages
100.000000 to 100.000000
version result total
8169350 ok 5937457
version total_tests
8169350 5937457
correctness_percentage
100.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants