[data] write_delta implementation - #64923
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a prototype implementation for writing Ray Datasets to Delta Lake tables via a new DeltaDatasink and a corresponding Dataset.write_delta method, supporting both append and overwrite modes. The review feedback highlights three key improvement opportunities: first, ensuring that storage_options are correctly propagated to the PyArrow FileSystem on workers to prevent write failures on secured cloud storage; second, optimizing empty APPEND operations to return early as a no-op instead of raising errors or committing empty transactions; and third, robustifying the partition parsing logic to correctly handle URL-encoded keys and map PyArrow's default null partition string (__HIVE_DEFAULT_PARTITION__) to None.
02c9fe8 to
a364b9e
Compare
Ports a small, independently-authored prototype write_delta implementation for a side-by-side comparison against the standalone write_delta design on stack2/pr1-write-delta-core. Dataset.write_delta and the DeltaDatasink class are kept as close to the shared original as possible -- deliberate fixes are called out below; everything else is unmodified -- so the diff between the two stacks reflects genuine design differences, not incidental ones. Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata. Bug fix 1 (found while building a write_delta release benchmark on top of this branch): a Dataset sourced from a file-listing-based read (e.g. read_parquet) failed every write with "Schema error: Invalid data type for Delta Lake: Null". Root cause: on_write_start's schema argument can be Ray Data's internal file-listing metadata schema (e.g. a null-typed __file_chunk_metadata column) rather than the dataset's actual row schema, and _resolve_schema unconditionally preferred that captured schema over the real schema workers observed while writing. Fixed by having write() return the schema of the Arrow table it actually wrote, and having _resolve_schema prefer that over the driver-side capture, which now serves only its originally-intended purpose: a fallback for a genuinely empty dataset with no worker-observed schema at all. Addresses review feedback from cursor[bot] and gemini-code-assist[bot] on PR ray-project#64923: - Worker tasks now honor storage_options when building their Parquet write filesystem (previously only the driver's DeltaTable/ create_table_with_add_actions calls received it, so a cloud write with explicit credentials could succeed on the driver's commit while every worker's file write silently used different, ambient credentials, or failed outright). - An empty-dataset APPEND against an already-existing table is now a pure no-op -- no schema resolution, no empty commit added to the Delta log. APPEND against a not-yet-existing table with no resolvable schema still raises (a brand-new table can't be created without knowing its shape); OVERWRITE is unaffected either way (an empty overwrite must still truncate the table). - Partition value/key parsing: a NULL partition value (written by PyArrow's Hive writer as the literal directory name "__HIVE_DEFAULT_PARTITION__") now round-trips back as Python None instead of that literal string; partition column names are now URL-unquoted the same way values already were. Adds a new test suite (test_write_delta.py) covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes, table name/description metadata, the empty-dataset error path (and the empty-APPEND-against-existing-table no-op path), storage_options passthrough to both the commit calls and the worker filesystem, null/URL-encoded partition value and key handling, and a regression test for the read_parquet-sourced schema bug above. One pyproject.toml per-file-ignore added (F401) to keep the ported file lint-clean without editing it for that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Abhishek Verma <abhishek.verma@anyscale.com>
a364b9e to
3c48d6b
Compare
Ports a small, independently-authored prototype write_delta implementation for a side-by-side comparison against the standalone write_delta design on stack2/pr1-write-delta-core. Dataset.write_delta and the DeltaDatasink class are kept as close to the shared original as possible -- deliberate fixes are called out below; everything else is unmodified -- so the diff between the two stacks reflects genuine design differences, not incidental ones. Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata. Bug fix 1 (found while building a write_delta release benchmark on top of this branch): a Dataset sourced from a file-listing-based read (e.g. read_parquet) failed every write with "Schema error: Invalid data type for Delta Lake: Null". Root cause: on_write_start's schema argument can be Ray Data's internal file-listing metadata schema (e.g. a null-typed __file_chunk_metadata column) rather than the dataset's actual row schema, and _resolve_schema unconditionally preferred that captured schema over the real schema workers observed while writing. Fixed by having write() return the schema of the Arrow table(s) it actually wrote, and having _resolve_schema prefer that over the driver-side capture, which now serves only its originally-intended purpose: a fallback for a genuinely empty dataset with no worker-observed schema at all. Addresses review feedback from cursor[bot] and gemini-code-assist[bot] on PR ray-project#64923, across two review rounds: - Worker tasks now honor storage_options when building their Parquet write filesystem (previously only the driver's DeltaTable/ create_table_with_add_actions calls received it, so a cloud write with explicit credentials could succeed on the driver's commit while every worker's file write silently used different, ambient credentials, or failed outright). - An empty-dataset APPEND against an already-existing table is now a pure no-op -- no schema resolution, no empty commit added to the Delta log. APPEND against a not-yet-existing table with no resolvable schema still raises (a brand-new table can't be created without knowing its shape); OVERWRITE is unaffected either way (an empty overwrite must still truncate the table). - Partition value/key parsing: a NULL partition value (written by PyArrow's Hive writer as the literal directory name "__HIVE_DEFAULT_PARTITION__") now round-trips back as Python None instead of that literal string; partition column names are now URL-unquoted the same way values already were. - write() now unifies the schema across every non-empty block a single task writes, instead of keeping only the last block's schema (a task can receive multiple blocks with promotable but non-identical schemas). - The worker-side Azure filesystem helper now reads the SAS token from AZURE_STORAGE_SAS_TOKEN, matching ray.data.catalog's vended-credential key, instead of a different name that would never match. - The worker-side GCS filesystem helper no longer leaves GOOGLE_APPLICATION_CREDENTIALS mutated after use -- the env var is a process-wide value that could otherwise leak a task's credential path to other tasks reusing the same warm Ray worker. Adds a new test suite (test_write_delta.py) covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes (including cross-block schema unification within a single task), table name/description metadata, the empty-dataset error path (and the empty-APPEND-against-existing-table no-op path), storage_options passthrough to both the commit calls and the worker filesystem (S3, Azure, and GCS, including the GCS env-var restore behavior), null/URL-encoded partition value and key handling, and a regression test for the read_parquet-sourced schema bug above. One pyproject.toml per-file-ignore added (F401) to keep the ported file lint-clean without editing it for that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Abhishek Verma <abhishek.verma@anyscale.com>
3c48d6b to
536ee28
Compare
Ports a small, independently-authored prototype write_delta implementation for a side-by-side comparison against the standalone write_delta design on stack2/pr1-write-delta-core. Dataset.write_delta and the DeltaDatasink class are kept as close to the shared original as possible -- deliberate fixes are called out below; everything else is unmodified -- so the diff between the two stacks reflects genuine design differences, not incidental ones. Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata. Bug fix 1 (found while building a write_delta release benchmark on top of this branch): a Dataset sourced from a file-listing-based read (e.g. read_parquet) failed every write with "Schema error: Invalid data type for Delta Lake: Null". Root cause: on_write_start's schema argument can be Ray Data's internal file-listing metadata schema (e.g. a null-typed __file_chunk_metadata column) rather than the dataset's actual row schema, and _resolve_schema unconditionally preferred that captured schema over the real schema workers observed while writing. Fixed by having write() return the schema of the Arrow table(s) it actually wrote, and having _resolve_schema prefer that over the driver-side capture, which now serves only its originally-intended purpose: a fallback for a genuinely empty dataset with no worker-observed schema at all. Addresses review feedback from cursor[bot] and gemini-code-assist[bot] on PR ray-project#64923, across three review rounds: - Worker tasks now honor storage_options when building their Parquet write filesystem (previously only the driver's DeltaTable/ create_table_with_add_actions calls received it, so a cloud write with explicit credentials could succeed on the driver's commit while every worker's file write silently used different, ambient credentials, or failed outright). - An empty-dataset APPEND against an already-existing table is now a pure no-op -- no schema resolution, no empty commit added to the Delta log. APPEND against a not-yet-existing table with no resolvable schema still raises (a brand-new table can't be created without knowing its shape); OVERWRITE is unaffected either way (an empty overwrite must still truncate the table). - Partition value/key parsing: a NULL partition value (written by PyArrow's Hive writer as the literal directory name "__HIVE_DEFAULT_PARTITION__") now round-trips back as Python None instead of that literal string; partition column names are now URL-unquoted the same way values already were. - write() now unifies the schema across every non-empty block a single task writes, instead of keeping only the last block's schema (a task can receive multiple blocks with promotable but non-identical schemas). - The worker-side Azure filesystem helper now reads the SAS token from AZURE_STORAGE_SAS_TOKEN, matching ray.data.catalog's vended-credential key, instead of a different name that would never match. - The worker-side GCS filesystem helper no longer leaves GOOGLE_APPLICATION_CREDENTIALS mutated after use -- the env var is a process-wide value that could otherwise leak a task's credential path to other tasks reusing the same warm Ray worker. - An empty-dataset OVERWRITE against an already-existing table now falls back to that table's own current schema instead of raising: truncating an existing table is a well-defined operation even when the incoming write has no rows (and therefore no worker-observed schema) to resolve one from. Adds a new test suite (test_write_delta.py) covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes (including cross-block schema unification within a single task), table name/description metadata, the empty-dataset error path (and the empty-APPEND-against-existing-table and empty-OVERWRITE-against-existing-table no-op/truncate paths), storage_options passthrough to both the commit calls and the worker filesystem (S3, Azure, and GCS, including the GCS env-var restore behavior), null/URL-encoded partition value and key handling, and a regression test for the read_parquet-sourced schema bug above. One pyproject.toml per-file-ignore added (F401) to keep the ported file lint-clean without editing it for that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Abhishek Verma <abhishek.verma@anyscale.com>
536ee28 to
d23ff6f
Compare
Ports a small, independently-authored prototype write_delta implementation for a side-by-side comparison against the standalone write_delta design on stack2/pr1-write-delta-core. Dataset.write_delta and the DeltaDatasink class are kept as close to the shared original as possible -- deliberate fixes are called out below; everything else is unmodified -- so the diff between the two stacks reflects genuine design differences, not incidental ones. Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata. Bug fix 1 (found while building a write_delta release benchmark on top of this branch): a Dataset sourced from a file-listing-based read (e.g. read_parquet) failed every write with "Schema error: Invalid data type for Delta Lake: Null". Root cause: on_write_start's schema argument can be Ray Data's internal file-listing metadata schema (e.g. a null-typed __file_chunk_metadata column) rather than the dataset's actual row schema, and _resolve_schema unconditionally preferred that captured schema over the real schema workers observed while writing. Fixed by having write() return the schema of the Arrow table(s) it actually wrote, and having _resolve_schema prefer that over the driver-side capture, which now serves only its originally-intended purpose: a fallback for a genuinely empty dataset with no worker-observed schema at all. Addresses review feedback from cursor[bot] and gemini-code-assist[bot] on both PR ray-project#64923 (this branch) and PR ray-project#64925 (the release-benchmark branch stacked on top, whose cumulative diff includes this file), across four review rounds: - Worker tasks now honor storage_options when building their Parquet write filesystem (previously only the driver's DeltaTable/ create_table_with_add_actions calls received it, so a cloud write with explicit credentials could succeed on the driver's commit while every worker's file write silently used different, ambient credentials, or failed outright). - An empty-dataset APPEND against an already-existing table is now a pure no-op -- no schema resolution, no empty commit added to the Delta log. APPEND against a not-yet-existing table with no resolvable schema still raises (a brand-new table can't be created without knowing its shape). - An empty-dataset OVERWRITE against an already-existing table now falls back to that table's own current schema instead of raising: truncating an existing table is well-defined even when the incoming write has no rows (and therefore no worker-observed schema) to resolve one from. - Partition value/key parsing: a NULL partition value (written by PyArrow's Hive writer as the literal directory name "__HIVE_DEFAULT_PARTITION__") now round-trips back as Python None instead of that literal string; partition column names are now URL-unquoted the same way values already were. - write() now unifies the schema across every non-empty block a single task writes, instead of keeping only the last block's schema (a task can receive multiple blocks with promotable but non-identical schemas). - The worker-side Azure filesystem helper now reads the SAS token from AZURE_STORAGE_SAS_TOKEN, matching ray.data.catalog's vended-credential key, instead of a different name that would never match. - The worker-side GCS filesystem helper no longer leaves GOOGLE_APPLICATION_CREDENTIALS mutated after use -- the env var is a process-wide value that could otherwise leak a task's credential path to other tasks reusing the same warm Ray worker. - The written-file-to-table-root relative path computation now strips leading slashes from both sides before computing the relative path: some cloud filesystems can return a table root without a leading slash from FileSystem.from_uri while written-file paths from that same filesystem still carry one (verified against a real S3-compatible server), and that mismatch previously made the relative path walk upward instead of resolving correctly, corrupting the Delta log. - The worker-side S3 filesystem helper now also accepts AWS_DEFAULT_REGION (a common boto-style key) as a fallback when AWS_REGION isn't set, matching what the driver's commit path (which forwards the full storage_options dict to deltalake) can already honor. - Dataset.write_delta's ray_remote_args parameter is now typed Optional[Dict[str, Any]], matching its actual default of None. Adds a new test suite (test_write_delta.py) covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes (including cross-block schema unification within a single task), table name/description metadata, the empty-dataset error path (and the empty-APPEND-against-existing-table and empty-OVERWRITE-against-existing-table no-op/truncate paths), storage_options passthrough to both the commit calls and the worker filesystem (S3, Azure, and GCS, including the GCS env-var restore behavior and the AWS_DEFAULT_REGION fallback), null/URL-encoded partition value and key handling, a simulated cross-filesystem leading-slash mismatch in the written-file relative path computation, and a regression test for the read_parquet-sourced schema bug above. One pyproject.toml per-file-ignore added (F401) to keep the ported file lint-clean without editing it for that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Abhishek Verma <abhishek.verma@anyscale.com>
d23ff6f to
4ac8ace
Compare
Ports a small, independently-authored prototype write_delta implementation for a side-by-side comparison against the standalone write_delta design on stack2/pr1-write-delta-core. Dataset.write_delta and the DeltaDatasink class are kept as close to the shared original as possible -- deliberate fixes are called out below; everything else is unmodified -- so the diff between the two stacks reflects genuine design differences, not incidental ones. Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata. Bug fix 1 (found while building a write_delta release benchmark on top of this branch): a Dataset sourced from a file-listing-based read (e.g. read_parquet) failed every write with "Schema error: Invalid data type for Delta Lake: Null". Root cause: on_write_start's schema argument can be Ray Data's internal file-listing metadata schema (e.g. a null-typed __file_chunk_metadata column) rather than the dataset's actual row schema, and _resolve_schema unconditionally preferred that captured schema over the real schema workers observed while writing. Fixed by having write() return the schema of the Arrow table(s) it actually wrote, and having _resolve_schema prefer that over the driver-side capture, which now serves only its originally-intended purpose: a fallback for a genuinely empty dataset with no worker-observed schema at all. Bug fix 2 (found from an actual CI run of the release benchmark, build 101454): a large distributed write (TPCH SF1000, ~1000 concurrent write tasks against a brand-new table root) failed with "AWS Error SLOW_DOWN during PutObject operation" while writing Parquet files, before any commit was attempted. Root cause: pyarrow.dataset.write_dataset defaults to create_dir=True, which for an S3-backed filesystem issues a PutObject to a single marker key representing the table root directory (verified empirically -- S3 has no real directories, so this is purely a convention). With ~1000 tasks all racing to create that identical key against a cold prefix, the concurrent-PUT burst triggers AWS throttling for no functional benefit (object writes don't need a pre-existing directory marker). Fixed by passing create_dir=False specifically when the resolved filesystem is a cloud object store (S3FileSystem, GcsFileSystem, AzureFileSystem); local (and other real) filesystems still get create_dir=True since they genuinely need the directory to exist (verified separately that create_dir=False raises FileNotFoundError against a local path that doesn't exist yet). Addresses review feedback from cursor[bot] and gemini-code-assist[bot] on both PR ray-project#64923 (this branch) and PR ray-project#64925 (the release-benchmark branch stacked on top, whose cumulative diff includes this file), across four review rounds: - Worker tasks now honor storage_options when building their Parquet write filesystem (previously only the driver's DeltaTable/ create_table_with_add_actions calls received it, so a cloud write with explicit credentials could succeed on the driver's commit while every worker's file write silently used different, ambient credentials, or failed outright). - An empty-dataset APPEND against an already-existing table is now a pure no-op -- no schema resolution, no empty commit added to the Delta log. APPEND against a not-yet-existing table with no resolvable schema still raises (a brand-new table can't be created without knowing its shape). - An empty-dataset OVERWRITE against an already-existing table now falls back to that table's own current schema instead of raising: truncating an existing table is well-defined even when the incoming write has no rows (and therefore no worker-observed schema) to resolve one from. - Partition value/key parsing: a NULL partition value (written by PyArrow's Hive writer as the literal directory name "__HIVE_DEFAULT_PARTITION__") now round-trips back as Python None instead of that literal string; partition column names are now URL-unquoted the same way values already were. - write() now unifies the schema across every non-empty block a single task writes, instead of keeping only the last block's schema (a task can receive multiple blocks with promotable but non-identical schemas). - The worker-side Azure filesystem helper now reads the SAS token from AZURE_STORAGE_SAS_TOKEN, matching ray.data.catalog's vended-credential key, instead of a different name that would never match. - The worker-side GCS filesystem helper no longer leaves GOOGLE_APPLICATION_CREDENTIALS mutated after use -- the env var is a process-wide value that could otherwise leak a task's credential path to other tasks reusing the same warm Ray worker. - The written-file-to-table-root relative path computation now strips leading slashes from both sides before computing the relative path: some cloud filesystems can return a table root without a leading slash from FileSystem.from_uri while written-file paths from that same filesystem still carry one (verified against a real S3-compatible server), and that mismatch previously made the relative path walk upward instead of resolving correctly, corrupting the Delta log. - The worker-side S3 filesystem helper now also accepts AWS_DEFAULT_REGION (a common boto-style key) as a fallback when AWS_REGION isn't set, matching what the driver's commit path (which forwards the full storage_options dict to deltalake) can already honor. - Dataset.write_delta's ray_remote_args parameter is now typed Optional[Dict[str, Any]], matching its actual default of None. Adds a new test suite (test_write_delta.py) covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes (including cross-block schema unification within a single task), table name/description metadata, the empty-dataset error path (and the empty-APPEND-against-existing-table and empty-OVERWRITE-against-existing-table no-op/truncate paths), storage_options passthrough to both the commit calls and the worker filesystem (S3, Azure, and GCS, including the GCS env-var restore behavior and the AWS_DEFAULT_REGION fallback), null/URL-encoded partition value and key handling, a simulated cross-filesystem leading-slash mismatch in the written-file relative path computation, a regression test for the read_parquet-sourced schema bug above, and a regression test that create_dir is only skipped for cloud object-store filesystems. One pyproject.toml per-file-ignore added (F401) to keep the ported file lint-clean without editing it for that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Abhishek Verma <abhishek.verma@anyscale.com>
4ac8ace to
bf32d89
Compare
bf32d89 to
dec293e
Compare
dec293e to
3391c11
Compare
e15497d to
7656a77
Compare
7656a77 to
238683e
Compare
238683e to
081406f
Compare
081406f to
37512d8
Compare
37512d8 to
42f7150
Compare
42f7150 to
dc1f9e3
Compare
| """Capture the dataset schema before any files are written, and -- | ||
| when writing to an already-existing table with no explicit | ||
| ``partition_by`` -- inherit the table's actual partition columns. | ||
|
|
||
| Without this, writing to an already-partitioned table without | ||
| re-specifying ``partition_by`` writes files flat (no Hive partition | ||
| directories) with empty ``AddAction.partition_values``. Verified | ||
| empirically that Delta's reader derives a partitioned column's | ||
| value from that metadata, not from the (still-present) column in | ||
| the physical Parquet file -- so every row written this way reads | ||
| back with ``None`` for the partition column, silently losing the | ||
| real value entirely, not just breaking the on-disk layout. | ||
|
|
||
| This applies to ``OVERWRITE`` as much as to ``APPEND``: an | ||
| overwrite doesn't reset the table's partitioning either. Verified | ||
| empirically that ``create_write_transaction(partition_by=None)`` | ||
| leaves the table's ``partition_columns`` metadata intact, so an | ||
| overwrite that skipped this would leave the table in a | ||
| self-contradictory state -- metadata saying partitioned, files | ||
| written flat -- with the same silent value loss on read. Changing | ||
| an existing table's partition scheme isn't supported by this | ||
| prototype (``deltalake`` rejects a ``partition_by`` that differs | ||
| from the table's own), so inheriting is the only correct behavior | ||
| here, not merely the convenient one. | ||
|
|
||
| This has to happen here, before any write task runs, not deferred | ||
| to the driver's commit -- unlike schema evolution, which the | ||
| commit-time reconciliation can safely handle after the fact, | ||
| workers need to know the partition columns *before* writing their | ||
| Parquet files. |
There was a problem hiding this comment.
Can we make this human driven comments? Verified empirically for example is not needed. Let's make this succinct.
| tables: List["pa.Table"] = [] | ||
| for block in blocks: | ||
| pa_table = BlockAccessor.for_block(block).to_arrow() | ||
| if pa_table.num_rows > 0: | ||
| tables.append(pa_table) | ||
|
|
||
| if not tables: | ||
| return DeltaWriteResult(add_actions=[], schema=None) | ||
|
|
||
| # A single task can receive multiple non-empty blocks with promotable | ||
| # but non-identical schemas (e.g. int32 in one block, int64 in | ||
| # another -- or, e.g., one block missing a column another has) -- | ||
| # unify across all of them, not just the last one, and cast every | ||
| # block to that unified schema before writing. Without the cast, | ||
| # each block's Parquet file would keep its own original (possibly | ||
| # narrower) type while the schema reported to the driver -- and | ||
| # ultimately committed to the Delta log -- says the wider, unified | ||
| # type, leaving the log's declared schema inconsistent with what's | ||
| # actually on disk. Mirrors ParquetDatasink's identical per-task | ||
| # unify-then-cast pattern. | ||
| # | ||
| # ``_align_struct_fields`` (not ``reorder_columns_by_schema``) is | ||
| # what does the reordering here: a block that's simply missing a | ||
| # column another block in the same task has (not just a narrower | ||
| # type for a shared column) needs that column backfilled with | ||
| # nulls before the positional ``.cast(schema)`` below, which | ||
| # ``reorder_columns_by_schema`` can't do -- verified empirically |
There was a problem hiding this comment.
Need to write each block to a separate parquet file, collect schemas, and then on the driver unify_schemas
There was a problem hiding this comment.
Sure, will change this to write out the blocks without accumulating them (which was being done to unify schemas within worker - this part will be removed). unify_schemas at driver is happening currently which would remain the same.
There was a problem hiding this comment.
Done in d0affef. write() now writes each non-empty block straight to its own Parquet file and returns one schema per block; DeltaWriteResult.schema became schemas: List[pa.Schema] and the driver unifies across all workers.
I checked this doesn't regress the earlier bot findings about uncast files, by committing deliberately mismatched files directly: a null column against a string one, and a block missing a column another has, both read back correctly ([None, 'x']) with only the unified schema in the log — Delta reconciles per-file schemas on read. The one case the cast nominally covered, int32 vs int64, can't occur: unify_schemas rejects it before any cast would run.
| def _resolve_schema( | ||
| self, worker_schemas: List["pa.Schema"] | ||
| ) -> Optional["pa.Schema"]: | ||
| """Pick the schema to commit, preferring the schema(s) workers | ||
| actually wrote over the driver's pre-write estimate. | ||
|
|
||
| ``self._schema`` (captured in ``on_write_start``) is used only as a | ||
| fallback for a genuinely empty dataset (no worker wrote any rows) -- | ||
| that's its original purpose per the comment in ``__init__``. It must | ||
| not be preferred over real worker schemas: for a dataset sourced | ||
| from a file-listing-based read (e.g. ``read_parquet``), the schema | ||
| Ray Data passes to ``on_write_start`` can be internal file-listing | ||
| metadata (e.g. a null-typed ``__file_chunk_metadata`` column) rather | ||
| than the actual row schema, which ``deltalake`` rejects outright. | ||
| """ | ||
| import pyarrow as pa | ||
|
|
||
| if not worker_schemas: | ||
| return self._schema | ||
| if len(worker_schemas) == 1: | ||
| return worker_schemas[0] | ||
| return pa.unify_schemas(worker_schemas) |
There was a problem hiding this comment.
Don't need this function, just call pa.unify_schemas on the driver.
There was a problem hiding this comment.
use existing unify_schemas() from transform_pyarrow.py instead of this helper function.
There was a problem hiding this comment.
helper function _resolve_schema removed.
| """Reconcile ``schema`` (the schema being committed) against ``dt``'s | ||
| current table schema, per ``self._schema_mode``. | ||
|
|
||
| A column in ``schema`` that ``dt`` doesn't have is either added to | ||
| the table (``schema_mode="merge"``) or rejected with a clear error | ||
| (``schema_mode="error"``). A column both schemas already have, but | ||
| with an incompatible type (e.g. a string where the table has an | ||
| int64), always raises -- regardless of ``schema_mode``, since that's | ||
| never a safe automatic fix, only ever adding new columns is. | ||
|
|
||
| Without this check entirely, an extra column in ``schema`` would be | ||
| silently dropped from the written data, and a type-incompatible | ||
| column would commit successfully but leave the table unreadable on | ||
| the very next read -- both silent-failure modes verified empirically | ||
| and worse than either evolving the schema or raising loudly. | ||
|
|
||
| The type-compatibility check runs *before* any schema evolution, | ||
| not after: ``pa.unify_schemas`` only raises for genuine type | ||
| conflicts on columns both schemas already share, regardless of | ||
| whether one side has extra columns the other doesn't -- so it | ||
| already gives a correct answer without needing the table evolved | ||
| first. Checking first means a write that both adds a new column and | ||
| has an incompatible type on an existing column raises before | ||
| ``alter.add_columns`` ever runs, instead of committing a real, | ||
| permanent schema change to the table and only then rejecting the | ||
| write -- which would leave the table's schema altered despite the | ||
| write having failed. | ||
|
|
||
| Returns the ``DeltaTable`` to commit against: the same ``dt`` if the |
There was a problem hiding this comment.
Remove comment
| existing_schema = pa.schema(dt.schema().to_arrow()) | ||
|
|
||
| try: | ||
| pa.unify_schemas([existing_schema, schema]) |
There was a problem hiding this comment.
Let's use transform_pyarrow's unify_schemas instead
There was a problem hiding this comment.
Switched both call sites over. Worth noting for anyone touching this later: it has to stay at the default promote_types=False. At the default it still raises ArrowTypeError for int64-vs-string and re-raises pyarrow's original exception, so the except pa.ArrowTypeError incompatibility check is unaffected. With promote_types=True pyarrow switches to promote_options="permissive" and silently coerces string+int64 → string, which would quietly disable that check.
| dt.alter.add_columns(delta_schema.fields) | ||
| return DeltaTable(self._path, storage_options=self._storage_options) | ||
|
|
||
| def _write_parquet(self, table: "pa.Table", ctx: TaskContext) -> List["AddAction"]: |
There was a problem hiding this comment.
Just compare against https://delta-io.github.io/delta-rs/api/delta_writer/#deltalake.write_deltalake . If it's functionally equivalent, please use it. If not, then fine.
There was a problem hiding this comment.
write_deltalake(data=...) only accepts in-memory Arrow: it normalizes the input to a RecordBatchReader and hands it to Rust write() / write_to_deltalake(), both typed data: RecordBatchReader. It writes the Parquet itself in the calling process and never references AddAction — there's no parameter anywhere for committing already-written files. Since our workers write their own Parquet in parallel and send back only metadata, adopting it would mean routing every worker's data through the driver, or doing per-worker commits and losing the single atomic commit. create_table_with_add_actions / create_write_transaction are the only metadata-only commit APIs in the package, which is what we already use.
| def _parse_partition_values( | ||
| rel_path: str, partition_by: List[str] | ||
| ) -> Dict[str, Optional[str]]: | ||
| """Extract ``{col: value}`` from a hive-partitioned relative file path.""" | ||
| import urllib.parse | ||
|
|
||
| if not partition_by: | ||
| return {} | ||
| values: Dict[str, Optional[str]] = {} | ||
| for segment in rel_path.split("/")[:-1]: | ||
| if "=" in segment: | ||
| key, _, value = segment.partition("=") | ||
| unquoted_key = urllib.parse.unquote(key) | ||
| unquoted_value = urllib.parse.unquote(value) | ||
| values[unquoted_key] = ( | ||
| None | ||
| if unquoted_value == "__HIVE_DEFAULT_PARTITION__" | ||
| else unquoted_value | ||
| ) |
There was a problem hiding this comment.
Look at partitioning.py to see if the parser already exists.
There was a problem hiding this comment.
Looked — PathPartitionParser._parse_hive_path does exist and does work on a bare relative path with no filesystem access. Three gaps stop it being a drop-in here:
It unquotes values but not keys.
No HIVE_DEFAULT_PARTITION → None mapping; it returns the literal string, which would break the null-partition round-trip. That convention is Hive/Delta-specific and appears nowhere in partitioning.py.
It returns only the keys present in the path, with no {col: None} backfill for every requested partition_by column — and with field_names set it raises on a count/name mismatch rather than filling in.
dc1f9e3 to
d0affef
Compare
d0affef to
a5eccdd
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit a5eccdd. Configure here.
Ports a small, independently-authored prototype write_delta implementation for a side-by-side comparison against the standalone write_delta design on stack2/pr1-write-delta-core. Dataset.write_delta and the DeltaDatasink class are kept as close to the shared original as possible -- deliberate fixes are called out below; everything else is unmodified -- so the diff between the two stacks reflects genuine design differences, not incidental ones. Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata. Bug fix 1 (found while building a write_delta release benchmark on top of this branch): a Dataset sourced from a file-listing-based read (e.g. read_parquet) failed every write with "Schema error: Invalid data type for Delta Lake: Null". Root cause: on_write_start's schema argument can be Ray Data's internal file-listing metadata schema (e.g. a null-typed __file_chunk_metadata column) rather than the dataset's actual row schema, and _resolve_schema unconditionally preferred that captured schema over the real schema workers observed while writing. Fixed by having write() return the schema of the Arrow table(s) it actually wrote, and having _resolve_schema prefer that over the driver-side capture, which now serves only its originally-intended purpose: a fallback for a genuinely empty dataset with no worker-observed schema at all. Bug fix 2 (found from an actual CI run of the release benchmark, build 101454): a large distributed write (TPCH SF1000, ~1000 concurrent write tasks against a brand-new table root) failed with "AWS Error SLOW_DOWN during PutObject operation" while writing Parquet files, before any commit was attempted. Root cause: pyarrow.dataset.write_dataset defaults to create_dir=True, which for an S3-backed filesystem issues a PutObject to a single marker key representing the table root directory (verified empirically -- S3 has no real directories, so this is purely a convention). With ~1000 tasks all racing to create that identical key against a cold prefix, the concurrent-PUT burst triggers AWS throttling for no functional benefit (object writes don't need a pre-existing directory marker). Fixed by passing create_dir=False specifically when the resolved filesystem is a cloud object store (S3FileSystem, GcsFileSystem, AzureFileSystem); local (and other real) filesystems still get create_dir=True since they genuinely need the directory to exist (verified separately that create_dir=False raises FileNotFoundError against a local path that doesn't exist yet). Addresses review feedback from cursor[bot] and gemini-code-assist[bot] on both PR ray-project#64923 (this branch) and PR ray-project#64925 (the release-benchmark branch stacked on top, whose cumulative diff includes this file), across four review rounds: - Worker tasks now honor storage_options when building their Parquet write filesystem (previously only the driver's DeltaTable/ create_table_with_add_actions calls received it, so a cloud write with explicit credentials could succeed on the driver's commit while every worker's file write silently used different, ambient credentials, or failed outright). - An empty-dataset APPEND against an already-existing table is now a pure no-op -- no schema resolution, no empty commit added to the Delta log. APPEND against a not-yet-existing table with no resolvable schema still raises (a brand-new table can't be created without knowing its shape). - An empty-dataset OVERWRITE against an already-existing table now falls back to that table's own current schema instead of raising: truncating an existing table is well-defined even when the incoming write has no rows (and therefore no worker-observed schema) to resolve one from. - Partition value/key parsing: a NULL partition value (written by PyArrow's Hive writer as the literal directory name "__HIVE_DEFAULT_PARTITION__") now round-trips back as Python None instead of that literal string; partition column names are now URL-unquoted the same way values already were. - write() now unifies the schema across every non-empty block a single task writes, instead of keeping only the last block's schema (a task can receive multiple blocks with promotable but non-identical schemas). - The worker-side Azure filesystem helper now reads the SAS token from AZURE_STORAGE_SAS_TOKEN, matching ray.data.catalog's vended-credential key, instead of a different name that would never match. - The worker-side GCS filesystem helper no longer leaves GOOGLE_APPLICATION_CREDENTIALS mutated after use -- the env var is a process-wide value that could otherwise leak a task's credential path to other tasks reusing the same warm Ray worker. - The written-file-to-table-root relative path computation now strips leading slashes from both sides before computing the relative path: some cloud filesystems can return a table root without a leading slash from FileSystem.from_uri while written-file paths from that same filesystem still carry one (verified against a real S3-compatible server), and that mismatch previously made the relative path walk upward instead of resolving correctly, corrupting the Delta log. - The worker-side S3 filesystem helper now also accepts AWS_DEFAULT_REGION (a common boto-style key) as a fallback when AWS_REGION isn't set, matching what the driver's commit path (which forwards the full storage_options dict to deltalake) can already honor. - Dataset.write_delta's ray_remote_args parameter is now typed Optional[Dict[str, Any]], matching its actual default of None. Adds a new test suite (test_write_delta.py) covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes (including cross-block schema unification within a single task), table name/description metadata, the empty-dataset error path (and the empty-APPEND-against-existing-table and empty-OVERWRITE-against-existing-table no-op/truncate paths), storage_options passthrough to both the commit calls and the worker filesystem (S3, Azure, and GCS, including the GCS env-var restore behavior and the AWS_DEFAULT_REGION fallback), null/URL-encoded partition value and key handling, a simulated cross-filesystem leading-slash mismatch in the written-file relative path computation, a regression test for the read_parquet-sourced schema bug above, and a regression test that create_dir is only skipped for cloud object-store filesystems. One pyproject.toml per-file-ignore added (F401) to keep the ported file lint-clean without editing it for that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Abhishek Verma <abhishek.verma@anyscale.com>
a5eccdd to
81ea505
Compare
Ports a small, independently-authored prototype write_delta implementation for a side-by-side comparison against the standalone write_delta design on stack2/pr1-write-delta-core. Dataset.write_delta and the DeltaDatasink class are kept as close to the shared original as possible -- deliberate fixes are called out below; everything else is unmodified -- so the diff between the two stacks reflects genuine design differences, not incidental ones. Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata. Bug fix 1 (found while building a write_delta release benchmark on top of this branch): a Dataset sourced from a file-listing-based read (e.g. read_parquet) failed every write with "Schema error: Invalid data type for Delta Lake: Null". Root cause: on_write_start's schema argument can be Ray Data's internal file-listing metadata schema (e.g. a null-typed __file_chunk_metadata column) rather than the dataset's actual row schema, and _resolve_schema unconditionally preferred that captured schema over the real schema workers observed while writing. Fixed by having write() return the schema of the Arrow table(s) it actually wrote, and having _resolve_schema prefer that over the driver-side capture, which now serves only its originally-intended purpose: a fallback for a genuinely empty dataset with no worker-observed schema at all. Bug fix 2 (found from an actual CI run of the release benchmark, build 101454): a large distributed write (TPCH SF1000, ~1000 concurrent write tasks against a brand-new table root) failed with "AWS Error SLOW_DOWN during PutObject operation" while writing Parquet files, before any commit was attempted. Root cause: pyarrow.dataset.write_dataset defaults to create_dir=True, which for an S3-backed filesystem issues a PutObject to a single marker key representing the table root directory (verified empirically -- S3 has no real directories, so this is purely a convention). With ~1000 tasks all racing to create that identical key against a cold prefix, the concurrent-PUT burst triggers AWS throttling for no functional benefit (object writes don't need a pre-existing directory marker). Fixed by passing create_dir=False specifically when the resolved filesystem is a cloud object store (S3FileSystem, GcsFileSystem, AzureFileSystem); local (and other real) filesystems still get create_dir=True since they genuinely need the directory to exist (verified separately that create_dir=False raises FileNotFoundError against a local path that doesn't exist yet). Addresses review feedback from cursor[bot] and gemini-code-assist[bot] on both PR ray-project#64923 (this branch) and PR ray-project#64925 (the release-benchmark branch stacked on top, whose cumulative diff includes this file), across four review rounds: - Worker tasks now honor storage_options when building their Parquet write filesystem (previously only the driver's DeltaTable/ create_table_with_add_actions calls received it, so a cloud write with explicit credentials could succeed on the driver's commit while every worker's file write silently used different, ambient credentials, or failed outright). - An empty-dataset APPEND against an already-existing table is now a pure no-op -- no schema resolution, no empty commit added to the Delta log. APPEND against a not-yet-existing table with no resolvable schema still raises (a brand-new table can't be created without knowing its shape). - An empty-dataset OVERWRITE against an already-existing table now falls back to that table's own current schema instead of raising: truncating an existing table is well-defined even when the incoming write has no rows (and therefore no worker-observed schema) to resolve one from. - Partition value/key parsing: a NULL partition value (written by PyArrow's Hive writer as the literal directory name "__HIVE_DEFAULT_PARTITION__") now round-trips back as Python None instead of that literal string; partition column names are now URL-unquoted the same way values already were. - write() now unifies the schema across every non-empty block a single task writes, instead of keeping only the last block's schema (a task can receive multiple blocks with promotable but non-identical schemas). - The worker-side Azure filesystem helper now reads the SAS token from AZURE_STORAGE_SAS_TOKEN, matching ray.data.catalog's vended-credential key, instead of a different name that would never match. - The worker-side GCS filesystem helper no longer leaves GOOGLE_APPLICATION_CREDENTIALS mutated after use -- the env var is a process-wide value that could otherwise leak a task's credential path to other tasks reusing the same warm Ray worker. - The written-file-to-table-root relative path computation now strips leading slashes from both sides before computing the relative path: some cloud filesystems can return a table root without a leading slash from FileSystem.from_uri while written-file paths from that same filesystem still carry one (verified against a real S3-compatible server), and that mismatch previously made the relative path walk upward instead of resolving correctly, corrupting the Delta log. - The worker-side S3 filesystem helper now also accepts AWS_DEFAULT_REGION (a common boto-style key) as a fallback when AWS_REGION isn't set, matching what the driver's commit path (which forwards the full storage_options dict to deltalake) can already honor. - Dataset.write_delta's ray_remote_args parameter is now typed Optional[Dict[str, Any]], matching its actual default of None. Adds a new test suite (test_write_delta.py) covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes (including cross-block schema unification within a single task), table name/description metadata, the empty-dataset error path (and the empty-APPEND-against-existing-table and empty-OVERWRITE-against-existing-table no-op/truncate paths), storage_options passthrough to both the commit calls and the worker filesystem (S3, Azure, and GCS, including the GCS env-var restore behavior and the AWS_DEFAULT_REGION fallback), null/URL-encoded partition value and key handling, a simulated cross-filesystem leading-slash mismatch in the written-file relative path computation, a regression test for the read_parquet-sourced schema bug above, and a regression test that create_dir is only skipped for cloud object-store filesystems. One pyproject.toml per-file-ignore added (F401) to keep the ported file lint-clean without editing it for that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Abhishek Verma <abhishek.verma@anyscale.com>
81ea505 to
1e3b5ae
Compare

Supports SaveMode.APPEND and SaveMode.OVERWRITE (a prototype scope, by design): workers write Parquet files via pyarrow.dataset.write_dataset and report AddAction metadata; the driver commits all files in one Delta transaction via the deltalake library, creating the table if it doesn't exist. Also supports partition_by (Hive-style), storage_options, and optional name/description table metadata.
Adds a new test suite covering what this implementation actually supports: APPEND/OVERWRITE round-trips, unsupported-mode rejection, single- and multi-column partitioning, multi-block writes, table name/description metadata, the empty-dataset error path, and storage_options passthrough to the commit calls.