Support copy from and insert..select pushdown with identity and time partitioning#322
Support copy from and insert..select pushdown with identity and time partitioning#322sfc-gh-mslot wants to merge 1 commit into
Conversation
8ff6221 to
986731f
Compare
sfc-gh-okalaci
left a comment
There was a problem hiding this comment.
Ok, I see that you are already pushing changes, and some of them already fixed. Still sharing as fyi
ab93e6b to
f0835c2
Compare
87789c2 to
debe2e9
Compare
| ICEBERG_OOR_NONE, | ||
| false /* wrapNativeIntervals */ ); | ||
| false /* wrapNativeIntervals */ , | ||
| NIL /* partitionByExprs */ ); |
There was a problem hiding this comment.
So, VACUUM will spilt data files which are impacted by partitioned writes skipping target_file_size_mb ? Say, a partitioned COPY wrote 100GB file, we'll spilt it into 200 files each 512MB.
That's probably the right action, we want VACUUM to fix it, but I wanted to raise it here:
- To make sure you agree and happy about it
- Maybe add a comment for future readers
There was a problem hiding this comment.
Hmz, that's a good point. This will affect large data dumps.
There was a problem hiding this comment.
I was checking if we can implement partition_by + file_size_bytes on DuckDB, and it seems they landed a PR last week, which doesn't solve it but references it as Future Work: duckdb/duckdb#22225 (comment)
Well, that's both good and bad. We cannot implement it for the current DuckDB version (or very hard), but we can (a) wait for the DuckDB maintainers to implement (b) or consider working on that as a patch here, and then send it over to DuckDB upstream.
|
Maybe one final note: We don't seem to have any tests for partition evolution. I don't see any risks, but would be good to say start with no partition, then |
a74d802 to
1e114fd
Compare
…tables Partitioned writes previously went through the row-by-row PartitionedDestReceiver, which routes each row to a per-partition CSV file before converting to Parquet. This change enables DuckDB's PARTITION_BY in COPY TO for tables using pushdownable transforms (identity, year, month, day, hour), letting DuckDB split the data in a single pass. Bucket and truncate transforms continue to use the existing path. Key changes: - Add partition_pushdown.c with transform-to-DuckDB-SQL conversion, query wrapping with synthetic partition columns, and Hive-style path parsing for partition values - Extend WriteQueryResultTo with partitionByExprs parameter that wraps the query with synthetic columns AFTER validation/interval wrappers and adds PARTITION_BY to the COPY command - Modify AddQueryResultToTable to detect pushdownable partitions, pass expressions through, and parse per-file partition values from DuckDB output paths - Lift blanket partition blocks in IsPushdownableInsertSelectQuery and IsCopyFromPushdownable to allow pushdown when all transforms are supported - Disable FILE_SIZE_BYTES when PARTITION_BY is used (DuckDB limitation) Signed-off-by: Marco Slot <marco.slot@snowflake.com>
64b419d to
3ab641e
Compare
|
Are there any concerns about the data values as path keys (null bytes, slashes etc)? (I haven't read in detail this PR or previous feedback, so I assume there may already be handled via some sort of escaping method, but it was the first thing I thought of when reviewing the PR description.) |
This PR adds support for pushing down INSERT..SELECT and COPY..FROM when the target table is partitioned using identity or time functions. It uses the PARTITION_BY clause in the COPY TO command in DuckDB to generate paths, using a synthetic column that contains the partition value.
Example:
writes files like:
where the partition value is obtained by parsing the value after __part_0 (the synthetic column name)