Skip to content

make zstd optional in sstable #2633

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

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

make zstd optional in sstable #2633

wants to merge 6 commits into from

Conversation

Parth
Copy link

@Parth Parth commented May 3, 2025

@Parth Parth marked this pull request as ready for review May 3, 2025 21:06
@PSeitz
Copy link
Collaborator

PSeitz commented May 4, 2025

I would prefer a separate feature flag columnar-zstd-compression with some comments in Cargo.toml

@Parth
Copy link
Author

Parth commented May 4, 2025

@PSeitz Do you mean sstable-zstd-compression?

Also do you want zstd-compression at the tantivy / columnar level to enable this automatically?

I'll fix the fmt as well

@Parth
Copy link
Author

Parth commented May 4, 2025

I think I see what you wanted so I pushed a commit. Happy to iterate if that's not exactly right just lmk. Also pushed a cargo +nightly fmt.

@PSeitz
Copy link
Collaborator

PSeitz commented May 4, 2025

I think I see what you wanted so I pushed a commit. Happy to iterate if that's not exactly right just lmk. Also pushed a cargo +nightly fmt.

Yes, for a tantivy user the sstable is more like an implementation detail, unlike the columnar storage.

CHANGELOG.md Outdated
@@ -2,6 +2,9 @@ Tantivy 0.23 - Unreleased
================================
Tantivy 0.23 will be backwards compatible with indices created with v0.22 and v0.21. The new minimum rust version will be 1.75.

> [!WARNING]
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's already outdated with the last release

https://github.com/quickwit-oss/tantivy/pull/2621/files

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I understand: I didn't update the lines linked in that pr so even if we do 0.24 I'm not sure I see the problem.

I could rebase but I don't think that's what you mean because that pr isn't merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

0.24 is already released, but the CHANGELOG pr is not yet merged

[features]
unstable = []
zstd-compression = ["sstable/zstd-compression"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

that should be a default feature flag

Copy link
Author

Choose a reason for hiding this comment

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

Are you saying I should make it a default flag and then eliminate the changelog entry so we can merge this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes

Copy link
Author

Choose a reason for hiding this comment

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

understood, pushed

@Parth Parth requested a review from PSeitz May 4, 2025 14:22
@Parth
Copy link
Author

Parth commented May 7, 2025

@trinity-1686a / @PSeitz lmk if there's anything else I can do to bring this closer to being merged and ultimately released

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