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

Add convenience wrapper for pragma optimize #572

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

alexcwatt
Copy link
Contributor

@alexcwatt alexcwatt commented Oct 29, 2024

The SQLite docs recommend some cases where PRAGMA optimize; should be used: https://www.sqlite.org/pragma.html#pragma_optimize

I have discovered recently a case where this helped a project at work (https://twitter.com/alexcwatt/status/1851094243323912631) and now I'm wondering about how to make this easier to discover and use.

Do we want a way to test that these wrappers translate to the expected execute call? I wasn't sure what tests to write for this change.

@flavorjones
Copy link
Member

flavorjones commented Oct 30, 2024

@alexcwatt Thanks for this! The omission was mentioned by @rickhull in #559

If we're going to allow the optimize bits to be passed in by the caller, I'd like the API to be a bit more developer-friendly. Would you consider defining some constants in lib/sqlite3/constants.rb?

Our test coverage is pretty bad right now for the pragma methods, so as long as we keep the implementation simple, I think it's probably OK to not have test coverage (or to use a mock to test that the method calls what it should).

@@ -338,6 +338,14 @@ def mmap_size=(size)
set_int_pragma "mmap_size", size
end

def optimize(bitmask = nil)
if bitmask
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see these bits made available as constants like many of the other sqlite bitmasks in lib/sqlite3/constants.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I took a pass at this; please let me know what you think.

Copy link
Member

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

Looks good! I added a DEFAULT constant and some tests.

@flavorjones
Copy link
Member

And I rebased off origin/main.

@flavorjones flavorjones merged commit c5d1d5c into sparklemotion:main Oct 31, 2024
105 checks passed
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.

2 participants