-
Notifications
You must be signed in to change notification settings - Fork 130
Chore: feature flag lance #4938
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
921d819 to
a848f31
Compare
Benchmarks: Random AccessSummary
|
Benchmarks: FineWebSummary
Detailed Results Table
|
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Benchmarks: CompressionSummary
Detailed Results Table
|
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
c097e6f to
6a61efa
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
6a61efa to
96f1f19
Compare
| Format::OnDiskVortex => "vortex", | ||
| Format::VortexCompact => "vortex", | ||
| Format::OnDiskDuckDB => "duckdb", | ||
| #[cfg(feature = "lance")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually looking again at the Format::Lance matches, I'm getting more biased towards not feature flagging the variant.
0ax1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think though that it is worth considering not feature flagging the enum variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't look, but LGTM
Also modifies the actions so that we only run lance benchmarks in 2 places: after a PR is merged and during nightly benchmarks.
Also let's hold off on merging this until tomorrow just in case it breaks something on the website.
Also note that the benchmark summaries on github (below) are going to be off since they no longer include lance