Open
Conversation
Merging this PR will degrade performance by 14.64%
Performance Changes
Comparing 
Footnotes
|
github-actions
bot
removed
the
action/benchmark
Contributor
Benchmarks: FineWeb NVMeSummary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (1.024x ➖, 1↑ 2↓)
datafusion / vortex-compact (0.979x ➖, 0↑ 0↓)
datafusion / parquet (0.940x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.982x ➖, 1↑ 1↓)
duckdb / vortex-compact (0.961x ➖, 1↑ 0↓)
duckdb / parquet (0.972x ➖, 0↑ 0↓)
Full attributed analysis
|
Contributor
|
The codspeed regression is real imo. I think we need to avoid validation in almost all cases tbh.... |
gatesn
requested changes
Mar 10, 2026
Contributor
gatesn
left a comment
gatesn
left a comment
There was a problem hiding this comment.
Let's discuss when we should run validation. Scalars are already slow, so it makes sense. Arrays it's less obvious...
Contributor
Polar Signals Profiling ResultsLatest Run
Previous Runs (3)
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals ProfilingSummary
datafusion / vortex-file-compressed (1.074x ➖, 0↑ 4↓)
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (1.002x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.001x ➖, 0↑ 0↓)
datafusion / parquet (0.994x ➖, 0↑ 0↓)
datafusion / arrow (0.973x ➖, 2↑ 0↓)
duckdb / vortex-file-compressed (0.988x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.004x ➖, 0↑ 0↓)
duckdb / parquet (0.962x ➖, 5↑ 1↓)
duckdb / duckdb (0.994x ➖, 0↑ 0↓)
Full attributed analysis
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (1.063x ➖, 0↑ 12↓)
datafusion / vortex-compact (0.925x ➖, 34↑ 0↓)
datafusion / parquet (1.055x ➖, 0↑ 8↓)
duckdb / vortex-file-compressed (0.953x ➖, 11↑ 0↓)
duckdb / vortex-compact (0.980x ➖, 2↑ 0↓)
duckdb / parquet (0.941x ➖, 17↑ 0↓)
duckdb / duckdb (0.961x ➖, 10↑ 1↓)
Full attributed analysis
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (0.897x ✅, 12↑ 0↓)
datafusion / vortex-compact (0.911x ➖, 6↑ 0↓)
datafusion / parquet (0.931x ➖, 3↑ 0↓)
datafusion / arrow (0.885x ✅, 16↑ 0↓)
duckdb / vortex-file-compressed (0.914x ➖, 5↑ 0↓)
duckdb / vortex-compact (0.927x ➖, 4↑ 0↓)
duckdb / parquet (0.966x ➖, 1↑ 0↓)
duckdb / duckdb (0.956x ➖, 1↑ 0↓)
Full attributed analysis
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (1.194x ➖, 0↑ 4↓)
datafusion / vortex-compact (1.115x ➖, 1↑ 4↓)
datafusion / parquet (0.971x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.039x ➖, 0↑ 1↓)
duckdb / vortex-compact (1.057x ➖, 0↑ 3↓)
duckdb / parquet (1.100x ➖, 0↑ 1↓)
Full attributed analysis
|
Contributor
Benchmarks: FineWeb S3Summary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (1.041x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.040x ➖, 0↑ 0↓)
datafusion / parquet (1.050x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.115x ➖, 0↑ 1↓)
duckdb / vortex-compact (1.047x ➖, 0↑ 1↓)
duckdb / parquet (1.092x ➖, 0↑ 0↓)
Full attributed analysis
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (1.003x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.005x ➖, 1↑ 2↓)
datafusion / parquet (1.032x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.054x ➖, 0↑ 1↓)
duckdb / vortex-compact (1.028x ➖, 0↑ 2↓)
duckdb / parquet (1.084x ➖, 0↑ 2↓)
Full attributed analysis
|
Contributor
Benchmarks: Random AccessSummary
unknown / unknown (0.892x ✅, 17↑ 0↓)
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
VerdictNo clear signal
Statistical Summary
duckdb / vortex-file-compressed (0.953x ➖, 1↑ 0↓)
duckdb / vortex-compact (0.977x ➖, 0↑ 0↓)
duckdb / parquet (0.978x ➖, 0↑ 0↓)
Full attributed analysis
|
Contributor
Benchmarks: Clickbench on NVMESummary
VerdictNo clear signal
Statistical Summary
datafusion / vortex-file-compressed (0.991x ➖, 2↑ 2↓)
datafusion / parquet (1.001x ➖, 1↑ 1↓)
duckdb / vortex-file-compressed (1.015x ➖, 2↑ 2↓)
duckdb / parquet (0.995x ➖, 0↑ 0↓)
duckdb / duckdb (1.049x ➖, 0↑ 4↓)
Full attributed analysis
|
Contributor
Benchmarks: CompressionSummary
unknown / unknown (0.993x ➖, 4↑ 2↓)
|
Contributor
Author
|
there are also failures on cuda due to @gatesn Note that we do have a |
connortsui20
force-pushed
the
ct/ext-validate-array
branch
from
5fc6c09 to
6bdb086
Compare
gatesn
requested changes
Mar 11, 2026
Contributor
gatesn
left a comment
gatesn
left a comment
There was a problem hiding this comment.
I think you need to go through all call sites of ExtensionArray::new and decide which should be unchecked, e.g. the compressor should be new_unchecked
connortsui20
force-pushed
the
ct/ext-validate-array
branch
from
22579ae to
a6653d9
Compare
connortsui20
added
the
action/benchmark
github-actions
bot
removed
the
action/benchmark
connortsui20
added
the
action/benchmark
connortsui20
force-pushed
the
ct/ext-validate-array
branch
from
36aa73a to
be8ffcf
Compare
Contributor
Author
|
so I'm running into cuda test failures where |
Contributor
|
|
connortsui20
removed
the
action/benchmark
connortsui20
force-pushed
the
ct/ext-validate-array
branch
from
be8ffcf to
6f8fea2
Compare
Contributor
Author
|
@robert3005 so the error is triggered during stats collection (of min max), is there a way to tell stats to copy the buffer to host? Is that even something that makes sense? |
Contributor
|
BufferHandle has |
Contributor
Author
|
So for me to do this, I have to figure out how to make this code: let stats = storage_array.statistics();
if let Some(min) = stats.compute_min::<i64>() {copy into a host buffer in order to compute the min stat. Do we need to fix that for the rest of the statistics, or is this just a bug? Shouldn't this compute function handle this internally? |
Contributor
|
@onursatici @0ax1 @joseph-isaacs whats the solution to Connor’s problem? I think we have some api to validate if whole array is on the host or not. We can use that to skip but not sure how to do optional validation. Long term we want to have compute function deal with it but exact behaviour of it is still undecided |
Contributor
|
I think we just document that validate will only be called for all-host arrays? Or you say that validation implementations should allow for non-host buffers and skip validation |
Contributor
|
In theory, constructing an array from disk should be new_unchecked because we shouldnt perform validation while reading a file. Similarly, any transformation / scalar function should assume correct behavior and not revalidate the array. So this is only really on the write path, which is all in-memory anyway. |
Contributor
|
I think what you suggest is fine, ie only validate all host arrays. We were adding debug assertions so you can run a slow gpu mode that still validate all the arrays for debugging |
Contributor
Author
|
Note that this issue is not limited to reading arrays from disk, for example if we decide to run an expression on an extension array (where really we run it on the storage array), that change might have invalidated the storage array. So I feel like this issue is inherent to stats computation on GPU buffers, not when and where we validate arrays. |
Contributor
|
I don't really understand this? I'm saying validation shouldn't be run on read And stats are all considered to be optional at the moment |
Contributor
Author
|
so basically this is a stats bug, not a validation issue (stats computation should not panic). Just so we can move forward since this is blocking some other things, should I just disable those specific tests? Also, something that I just learned is that if stats computation ERRORs, it flattens and gives back |
Contributor
|
You have I think computation error being None is consistent... Not sure what the caller would do if you gave them something else. |
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/ext-validate-array
branch
from
6f8fea2 to
a0122c4
Compare
Contributor
Author
Specifically for min and max, None has a double meaning:
I have no way to distinguish between these 2 things (without inspecting the array directly), I feel that this is concerning |
connortsui20
added
the
action/benchmark
github-actions
bot
removed
the
action/benchmark
Contributor
|
This PR still feels weird to me. I think it boils down to the fact that validation can do compute. Therefore it should take an ExecutionCtx.... therefore ExtensionArray::try_new takes an ExecutionCtx? Surely not. Maybe there is only a new_unchecked constructor? The ExtensionArrayBuilder can validate. And other call sites must provide their own guarantees. |
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.
Summary
Tracking Issue: #6872
Adds a
validate_arrayandarray_validatemethod toExtVTableandExtDTypeRef, respectively.This addition required adding implementations for all of the existing extension types (which is currently just the datetime types, UUID, and Tensor).
Also fixes some bugs in the implementation of the existing types, as well as some other tests that used to pass because we didn't validate anything before!
API Changes
Adds some new methods, so this doesn't break anything.
Testing
Adds a few tests via
ExtensionArray::try_new.