Repository navigation
DictStrategy probes a fixed row sample instead of the first chunk - #10927
Conversation
Merging this PR will not alter performance2 warnings
Comparing |
| Some(chunk) => { | ||
| Some(sample) => { | ||
| let mut exec_ctx = session.create_execution_ctx(); | ||
| let sample = sample.execute::<Canonical>(&mut exec_ctx)?.into_array(); |
There was a problem hiding this comment.
sample is Chunked or Sliced<Chunked> so need to execute.. I think?
Polar Signals Profiling ResultsLatest Run
Previous Runs (14)
Powered by Polar Signals Cloud |
Benchmarks: PolarSignals Profiling 📖Commits: PR datafusion / vortex-file-compressed / ns (1.004x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: FineWeb NVMe 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.979x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.999x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.010x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.005x ➖, 0↑ 0↓)
No file size changes detected. |
DictStrategy probes a fixed row sample instead of the first chunk
Signed-off-by: Matt Katz <[email protected]>
3874127 to
4986b12
Compare
Benchmarks: TPC-H SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.005x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.999x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.991x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.999x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: TPC-H SF=10 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.996x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.003x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.995x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.000x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: TPC-DS SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.002x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.002x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.002x ➖, 0↑ 1↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 1↓)
No file size changes detected. |
Benchmarks: Clickbench on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.990x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.994x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.987x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.982x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: TPC-H SF=1 on S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.162x ➖, 2↑ 5↓)
datafusion / parquet / ns (1.029x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed / ns (1.121x ➖, 0↑ 4↓)
duckdb / parquet / ns (1.009x ➖, 0↑ 1↓)
|
| /// front and its first `probe_len` rows, or `None` if the stream is empty. | ||
| async fn peek_probe_sample( | ||
| mut stream: BoxStream<'static, SequencedChunk>, | ||
| dtype: &DType, |
There was a problem hiding this comment.
do we need this change?
if we do shall we assert or use first chunk one?
| Some(chunk) => { | ||
| Some(sample) => { | ||
| let mut exec_ctx = session.create_execution_ctx(); | ||
| let sample = sample.execute::<Canonical>(&mut exec_ctx)?.into_array(); |
…the first chunk Signed-off-by: Matt Katz <[email protected]>
Summary
DictStrategydecides whether to dictionary-encode a column by test-compressing its first chunk. It sees a consistent sample only because the 8kRepartitionStrategyabove it always hands it one 8,192-row chunk. It now probes the firstprobe_lenrows, collected across chunks, so the decision no longer depends on how the input is chunked.Changes
DictLayoutOptions::probe_len(default 8,192).WriteStrategyBuildersets it to the row block size, so the decision matches today's for any row block size.peek_first_chunkbecomespeek_probe_sample: it peeks chunks until it hasprobe_lenrows, puts them back in front of the stream unchanged, and probes the canonicalized firstprobe_lenrows.