Feature: Migrate genomics benchmark to FixedSizeList#4605
Closed
connortsui20 wants to merge 1 commit into
Closed
Conversation
connortsui20
added
changelog/feature
github-actions
Bot
removed
the
action/benchmark-sql
CodSpeed Performance ReportMerging #4605 will not alter performanceComparing Summary
|
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Contributor
|
The other list-y science-y benchmark to consider is https://github.com/iris-hep/adl-benchmarks-index . I think there's fewer missing data but I'm not sure duckdb has all the primitives necessary for those queries. |
Contributor
Author
|
@claude review |
Contributor
|
Claude finished @connortsui20's task —— View job Code Review: PR #4605 - Feature: Migrate genomics benchmark to
|
Contributor
Author
|
I'm thinking we probably don't want to merge this change? Or at least not until we figure out a better way to compress sparse fixed-size list data (which might come up a lot more in scientific computing and mathematics, though maybe not so much in AI/ML workloads). |
connortsui20
force-pushed
the
ct/fsl-statpopgen
branch
from
b014f20 to
b849ec4
Compare
connortsui20
changed the base branch from
develop
to
ct/duckdb-fsl-export-fix
Also fixes a subtle bug in the duckdb exporter, where duckdb `ARRAY` is quite different from their `LIST` type. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/fsl-statpopgen
branch
from
b849ec4 to
a52a32f
Compare
Contributor
Author
|
Closing since I don't think this really gives us much of a benefit (unless we are strictly talking about performance versus Parquet fixed-size lists). CC @robert3005 |
Contributor
|
I was hoping it would be faster but I guess we can't efficiently compress these yet |
connortsui20
added a commit
that referenced
this pull request
Sep 18, 2025
Tracking Issue: #4372 Changes checked out from: #4605 Fixes a bug in the DuckDB exporter where if we do not flatten / materialize the inner child of the fixed-size list before exporting the vector to DuckDB, DuckDB will segfault after running a function like `scan`. Also fixes a bug where I didn't correctly append garbage values when the validity is all null (I took that from the list implementation, which doesn't need to append garbage values since null can be an empty list there). I don't actually know if this is an issue to be honest, but it seems wrong? I was not able to figure out how to write regression tests for both of these. The early return might not actually be an issue, but the flattening at the end definitely is. However, I was not able to create a minimally reproducible test for that, and it only fails in query 7 and 8 in `statpopgen.sql`. ```sql -- 7. Read just one variant (this is the sixth one). SELECT * FROM statpopgen WHERE "CHROM" == 'chr21' AND "POS" == 5030278; -- 8. Read a 700 base-pair window of variants. SELECT * FROM statpopgen WHERE "CHROM" == 'chr21' AND "POS" >= 5030300 AND "POS" <= 5031000; ``` If anyone has an idea of how to recreate this easily that would be appreciated! Finally, I simplified the tests I wrote from before since a lot were redundant. --------- Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracking Issue: #4372
Also fixes a subtle bug in the duckdb exporter, where duckdb
ARRAYis quite different from theirLISTtype.This was mostly a proof of concept to see if the
FixedSizeListholds up against some real data. It found a few things (#4592, #4601), but nothing else significant.@danking and I observed that for scale factor 1, where most of the data is null, this actually performs worse than the variable-size list arrays. Part of this may be due to the compressor being better able to compress list sequences and "two-level" numeric sequences in nested types.
Note however that fixed-size lists vs Parquet are a lot better (but we think that is because Parquet is just much worse with fixed-size lists because it has to store a bunch of null values before it's black box compression).
I would expect the fixed-size lists to be better than variable size lists on much more dense data.