Phase 1: Align the TPC-DS generation path with the TPC-H path - #400
Phase 1: Align the TPC-DS generation path with the TPC-H path#400qbacpey wants to merge 3 commits into
Conversation
|
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. |
- 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 |
There was a problem hiding this comment.
Need statistic from TPC-DS 1K & 3K generation process to adjust these number
paul-aiyedun
left a comment
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
| from generate_data_files import generate_data_files | ||
|
|
||
|
|
||
| def test_max_rows_per_file_splits_tables_tpcds(setup_and_teardown): |
There was a problem hiding this comment.
Please update the test case to cover both TPC-H and TPC-DS.
| 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. |
There was a problem hiding this comment.
Nit: Remove comment about parallel dsdgen or add a TODO for this.
|
|
||
| def get_column_projection(column_metadata): | ||
| col_name, col_type, *_ = column_metadata | ||
| if is_decimal_column(col_type): |
There was a problem hiding this comment.
Why do we always do a conversion here?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Can we write the metadata on a shared path (i.e. outside of generate_data_files_with_duckdb or generate_data_files_with_duckdb)?
| 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}" |
There was a problem hiding this comment.
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}?
There was a problem hiding this comment.
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'" |
There was a problem hiding this comment.
Can we move this to a function that is shared with _write_table_partitions?
| # 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. |
There was a problem hiding this comment.
Why is this second pass needed?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
How was this number derived?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Makes every test in codec_definitions_test.py run only with TPC-H (currently codec definitions are supported only for TPC-H)
TL;DR
This PR add these features for DuckDB TPC-DS datasets generations:
--approx-row-group-bytesis now supported with zero overhead. The error of every table but one lands within −5.9% to +7.1%.--max-rows-per-filesplit into<table>-<part>.parquet, matching the tpchgen layout.metadata.jsonrecordsapprox_row_group_bytesalongsidescale_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:
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%
inventoryis 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.