Skip to content

doc: add Database.LSMTree.Simple #653

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
Merged
merged 3 commits into from
Apr 7, 2025
Merged

doc: add Database.LSMTree.Simple #653

merged 3 commits into from
Apr 7, 2025

Conversation

wenkokke
Copy link
Collaborator

@wenkokke wenkokke commented Mar 27, 2025 •
edited
Loading

This PR is ready for review. The fixes to TableNotCompatibleError will be addressed once #660 is merged. In the meantime, this is ready for review and for merging. This PR has been reviewed by @jeltsch.

@wenkokke wenkokke marked this pull request as draft March 27, 2025 14:34
@wenkokke wenkokke mentioned this pull request Mar 27, 2025
Merged
@wenkokke wenkokke force-pushed the wenkokke/simple branch 3 times, most recently from 00f63c5 to 593e161 Compare March 28, 2025 17:07
@wenkokke wenkokke marked this pull request as ready for review March 28, 2025 17:07
@wenkokke wenkokke changed the title wip: add Database.LSMTree.Simple doc: add Database.LSMTree.Simple Mar 28, 2025
jorisdral
jorisdral previously requested changes Apr 1, 2025
View reviewed changes
Copy link
Collaborator

@jorisdral jorisdral left a comment

Choose a reason for hiding this comment

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

Very nice, I think the Simple module is a very good entry point for new users. There are some suggestions below:

@wenkokke
Copy link
Collaborator Author

wenkokke commented Apr 1, 2025

Very nice, I think the Simple module is a very good entry point for new users. There are some suggestions below:

Did you intend to attach some suggestions, or was this in reference to the review itself?

@jorisdral
Copy link
Collaborator

Very nice, I think the Simple module is a very good entry point for new users. There are some suggestions below:

Did you intend to attach some suggestions, or was this in reference to the review itself?

Ah, I was referencing the review itself

@wenkokke
Copy link
Collaborator Author

wenkokke commented Apr 2, 2025

@jorisdral Ready for re-review.

@wenkokke wenkokke force-pushed the wenkokke/simple branch 3 times, most recently from be74e03 to 96117c3 Compare April 2, 2025 14:41
@wenkokke
Copy link
Collaborator Author

wenkokke commented Apr 2, 2025

@jorisdral ready for re-review

@wenkokke wenkokke force-pushed the wenkokke/simple branch 2 times, most recently from c15a34e to ccf0e64 Compare April 2, 2025 15:04
jorisdral
jorisdral reviewed Apr 3, 2025
View reviewed changes
Copy link
Collaborator

@jorisdral jorisdral left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we can cut out some of the golden file noise

@wenkokke
Copy link
Collaborator Author

wenkokke commented Apr 3, 2025

LGTM, but I think we can cut out some of the golden file noise

That's why it's in its own commit, and is a direct consequence of adding the simple table type to the golden tests (as the primary table type). The golden test noise needs to happen eventually, and might as well happen now.

@jorisdral
Copy link
Collaborator

LGTM, but I think we can cut out some of the golden file noise

That's why it's in its own commit, and is a direct consequence of adding the simple table type to the golden tests (as the primary table type). The golden test noise needs to happen eventually, and might as well happen now.

See #653 (comment)

@wenkokke wenkokke force-pushed the wenkokke/simple branch 3 times, most recently from ba1fb00 to fa8c2e0 Compare April 4, 2025 14:42
@wenkokke wenkokke requested a review from jorisdral April 4, 2025 16:34
@wenkokke wenkokke dismissed jorisdral’s stale review April 4, 2025 16:36

Addressed all requested changes.

jorisdral
jorisdral approved these changes Apr 7, 2025
View reviewed changes
@wenkokke wenkokke enabled auto-merge April 7, 2025 11:52
@wenkokke wenkokke added this pull request to the merge queue Apr 7, 2025
Merged via the queue into main with commit d4e7ada Apr 7, 2025
31 checks passed
@wenkokke wenkokke deleted the wenkokke/simple branch April 7, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisdral jorisdral jorisdral approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts dcoutts is a code owner

@mheinzel mheinzel Awaiting requested review from mheinzel mheinzel is a code owner

@recursion-ninja recursion-ninja Awaiting requested review from recursion-ninja recursion-ninja is a code owner

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

Successfully merging this pull request may close these issues.
2 participants
@wenkokke @jorisdral