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.

Sign up for GitHub

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

Jump to bottom

Added stored procedure versions of all dolt functions #3246

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Added stored procedure versions of all dolt functions #3246

merged 1 commit into from
Apr 19, 2022

Conversation

Hydrocharged
Copy link
Contributor

No description provided.

zachmu
zachmu reviewed Apr 18, 2022
View reviewed changes
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Two main comments:

  1. Several of the migrated functions don't need to be. Anything that's read-only is actually functional and should be left as a function.

  2. The tests you've added would be better expressed as script tests in the engine. We do want bats coverage just as a smoke test, but generally speaking this is engine functionality and should be tested in golang. The only reason this wasn't the case from the beginning is that it wasn't possible to test custom functions in engine tests at first.

Other than that this is straightforward and seems fine.

go/libraries/doltcore/sqle/dfunctions/dolt_merge_base.go Outdated Show resolved Hide resolved
go/libraries/doltcore/sqle/dfunctions/hashof.go Outdated Show resolved Hide resolved
go/libraries/doltcore/sqle/dfunctions/verify_constraints.go Show resolved Hide resolved
go/libraries/doltcore/sqle/dfunctions/version.go Outdated Show resolved Hide resolved
go/libraries/doltcore/sqle/dfunctions/active_branch.go Outdated Show resolved Hide resolved
@Hydrocharged
Copy link
Contributor Author

Failed test is flaking? Only failing on MacOS, and none of my changes should have any impact on any of the code used in the failing test. Could rerun it until it passes, but going to go ahead and merge.

@Hydrocharged Hydrocharged merged commit 7fe4d7d into main Apr 19, 2022
@Hydrocharged Hydrocharged deleted the daylon/external-procedures branch April 19, 2022 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zachmu zachmu zachmu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Hydrocharged @zachmu