Skip to content

Phase 1: Align the TPC-DS generation path with the TPC-H path - #400

Draft
qbacpey wants to merge 3 commits into
rapidsai:mainfrom
qbacpey:duckdb-tpcds-rowgroup-sizing
Draft

Phase 1: Align the TPC-DS generation path with the TPC-H path#400
qbacpey wants to merge 3 commits into
rapidsai:mainfrom
qbacpey:duckdb-tpcds-rowgroup-sizing

Conversation

@qbacpey

@qbacpey qbacpey commented Aug 10, 2026

Copy link
Copy Markdown

TL;DR

This PR add these features for DuckDB TPC-DS datasets generations:

  • Row-group sizing: --approx-row-group-bytes is now supported with zero overhead. The error of every table but one lands within −5.9% to +7.1%.
  • Parquet V2: all output uses V2 encodings. This needs DuckDB moves 1.3.2 -> 1.5.5.
  • Multi-file partitioning: tables over --max-rows-per-file split into <table>-<part>.parquet, matching the tpchgen layout.
  • Metadata: both paths now share one writer, so metadata.json records approx_row_group_bytes alongside scale_factor.

Results

Measured end to end on lab machine, TPC-DS.

Overhead

"Probe wait" is the time the main thread blocked on the probe after materialization finished, i.e. the only part of it that is not hidden:

target SF materialize probe wait write total
SF1 10.9s 0.9s 4.8s 16.6s
SF10 99.7s 0.0s 24.5s 124.2s
SF30 227.8s 0.0s 36.6s 264.4s
SF100 801.2s 0.0s 436.0s 1237.2s

At SF1 the probe costs 0.9s because it generates the same scale factor as the target and so cannot finish first.

Accuracy

Most probing error lands within −5.9% to +7.1%

image

inventory is the one exception, it has a discontinuity the other tables do not; see appendix B.

Tests

All 15 tests pass with DuckDB 1.5.5.

The CI row-group test uses a 1 MiB target so that SF1 tables contain enough row groups to validate sizing. The larger-target accuracy matrix above comes from manual runs.

@copy-pr-bot

copy-pr-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@qbacpey qbacpey changed the title Phase 1: bring the DuckDB (TPC-DS) generation path in line with the TPC-H path Phase 1: bring TPC-DS generation path align with TPC-H path Aug 10, 2026
- Updated `init_benchmark_tables` to accept a DuckDB connection parameter, allowing for more flexible database interactions.
- Introduced `get_select_query` and `get_column_projection` functions to handle SQL queries and column type conversions, improving data handling in the generation process.
- Modified `generate_data_files_with_duckdb` to utilize the new connection parameter and updated logic for writing metadata and table partitions.
- Enhanced tests to validate the new functionality and ensure compatibility with both TPCH and TPCDS benchmarks.
import pyarrow.parquet as pq
from duckdb_utils import get_select_query, init_benchmark_tables

_ROW_GROUP_GRANULARITY = 2048 # DuckDB rounds ROW_GROUP_SIZE to its vector size

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Need statistic from TPC-DS 1K & 3K generation process to adjust these number

@qbacpey qbacpey changed the title Phase 1: bring TPC-DS generation path align with TPC-H path Phase 1: Align the TPC-DS generation path with the TPC-H path Aug 10, 2026

@paul-aiyedun paul-aiyedun left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes overall look good to me. However, I had a few questions and code cleanup comments.

def test_generated_files_use_v2_page_format(setup_and_teardown):
"""Verify every generated Parquet file is written with the v2 format."""
@pytest.mark.parametrize(
"benchmark_type,use_duckdb",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think the use_duckdb parameter is needed here. We can have the test execution be based on the default value. Also, can we apply the parameterization globally at the setup_and_teardown level, so that all tests cover TPC-DS?

Same comment applies to the row_group_size_test.py update.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

from generate_data_files import generate_data_files


def test_max_rows_per_file_splits_tables_tpcds(setup_and_teardown):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please update the test case to cover both TPC-H and TPC-DS.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

Comment thread benchmark_data_tools/requirements.txt Outdated
charset-normalizer==3.4.3
click==8.2.1
duckdb==1.3.2
# DuckDB 1.5.5 supports Parquet V2; parallel dsdgen is not yet in a stable release.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Remove comment about parallel dsdgen or add a TODO for this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed


def get_column_projection(column_metadata):
col_name, col_type, *_ = column_metadata
if is_decimal_column(col_type):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we always do a conversion here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The conversion is not unconditional. get_column_projection() is only called inside the convert_decimals_to_floats branch of get_select_query(). Without -c, get_select_query() returns SELECT *, so no casts are generated.

with open(f"{args.data_dir_path}/metadata.json", "w") as file:
json.dump({"scale_factor": args.scale_factor}, file, indent=2)
file.write("\n")
write_metadata(args)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we write the metadata on a shared path (i.e. outside of generate_data_files_with_duckdb or generate_data_files_with_duckdb)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

for part in range(num_partitions):
# Avoid a redundant LIMIT/OFFSET for a single part.
partition_query = (
f"{select_query} LIMIT {max_rows_per_file} OFFSET {part * max_rows_per_file}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you try comparing the performance with an implementation that runs this in parallel and does WHERE rowid >= {part * max_rows_per_file} AND rowid < {max_rows_per_file}?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Observed performance scaling roughly proportional to the number of threads. Also set the concurrency to max_workers = min(num_threads, num_partitions) to avoid excessive memory growth and I/O contention.



def _write_probe(conn, query, path, row_group_rows=None):
options = "FORMAT parquet, PARQUET_VERSION 'V2'"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move this to a function that is shared with _write_table_partitions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

# A second write cannot fill the estimated row group or improve the result.
return rows

# Second pass: measure one full row group near the requested size.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this second pass needed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Encoded bytes/row changes with row-group size, so the first pass only gives a rough estimate based on DuckDB’s default-sized groups. The second pass uses that estimate to write one complete group near the requested size and measures bytes/row again there.


_ROW_GROUP_GRANULARITY = 2048 # DuckDB rounds ROW_GROUP_SIZE to its vector size
_MAX_PROBE_SCALE_FACTOR = 10
_STAGE1_ROWS = 200_000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How was this number derived?

@qbacpey qbacpey Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

200,000 was originally chosen to ensure DuckDB wrote at least one complete row group. I will change it to 122,880 (DuckDB’s default row-group size).

_MAX_PROBE_SCALE_FACTOR = 10 is for MEM budget, plan to adjust this when able to run SF1K &3K generation.

TEST_NON_DEFAULT_COMPRESSION_PATH = TESTS_DIR / "test_codec_definitions_non_default_compression.json"
TEST_INVALID_COMPRESSION_PATH = TESTS_DIR / "test_codec_definitions_invalid_compression.json"

pytestmark = pytest.mark.parametrize("setup_and_teardown", ["tpch"], indirect=True)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Makes every test in codec_definitions_test.py run only with TPC-H (currently codec definitions are supported only for TPC-H)

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