[connectors] Soft deletes - #6747
Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. Clean wrapper design (SoftDeleteHandle covers every format and the integrated Arrow path uniformly), thorough tests (unit + integration across JSON/Debezium/Avro/CSV/Parquet/raw + Delta CDC end-to-end + Python Kafka), correct rejection of primary-key tables and output connectors, backward-compatible config (skip_serializing_if) with a checkpoint-compat test, actionable error messages, docs and changelog updated, well-written commit messages with no AI-attribution trailers.
The Describe Manual Test Plan box in the PR body is empty — noted only because the checkbox is marked; automated coverage more than compensates, so not a blocker.
| to recover the current contents of the stream by ranking the changes of each key | ||
| by time, keeping the most recent one, and returning it only when it is an insertion. | ||
| Order the changes by a timestamp that the source attaches to each change rather | ||
| than by a column of the record. The example below use the timestamp the Kafka connector |
There was a problem hiding this comment.
Nit: "The example below use" → "The example below uses".
| /// When `true`, a delete received by the connector is pushed to the table | ||
| /// as an insertion of the same record, and the connector attaches the | ||
| /// `is_delete` metadata attribute set to `true` to it. Insertions carry no | ||
| /// `is_delete` attribute, so a column declared as |
There was a problem hiding this comment.
this is a bit of a weird choice. Why not make it a non-nullable Boolean and fill a "false"?
There was a problem hiding this comment.
Performance. In the common case where deletes are rare, this avoid the overhead of injecting metadata in most records.
| /// | ||
| /// Fails if the table has a primary key: a deletion in such a table identifies | ||
| /// a key rather than a record, so there is nothing to insert in its place. | ||
| fn soft_delete_input_handle( |
There was a problem hiding this comment.
I guess there is something for the java compiler to validate, I can add an additional commit.
| // A version of Feldera that predates this property drops it silently, so | ||
| // record that it took effect: the absence of this line in the log is how an | ||
| // operator recognizes that case. | ||
| info!("endpoint '{endpoint_name}': ingesting deletions into table '{stream}' as insertions"); |
There was a problem hiding this comment.
This line can be confusing, maybe add "with is_delete = true"
| therefore holds the current contents of the input stream, and the records the | ||
| stream deleted are gone. | ||
|
|
||
| Setting `soft_delete` to `true` keeps them. The connector pushes every record it |
There was a problem hiding this comment.
converts the table into a log of all operations
| Setting `soft_delete` to `true` keeps them. The connector pushes every record it | ||
| reads to the table as an insertion, including the records the stream deletes, and | ||
| attaches the `is_delete` metadata attribute to the deleted ones. The table then | ||
| accumulates the entire history of the input stream, and a query can select the |
There was a problem hiding this comment.
"accumulates" suggests keeping it as state, which is not true.
The table represents the unbounded stream of updates
| } | ||
|
|
||
| // Inserted and deleted records in the batch need different metadata, | ||
| // but metadata applies to an entire batch, so split the batch in two. |
There was a problem hiding this comment.
only 2 always? They are sorted by operation?
There was a problem hiding this comment.
they are unordered, only because the table doesn't have a PK.
| } | ||
| } | ||
|
|
||
| impl InputBuffer for SoftDeleteArrowStream { |
There was a problem hiding this comment.
should all these structs be in the same file?
| //! Tests for the `soft_delete` connector property. | ||
| //! | ||
| //! Each test drives records through a real parser (or, for connectors that | ||
| //! bypass parsers, through the Arrow input stream) into a table whose |
| &[ | ||
| br#"{"payload": {"op": "c", "after": {"id": 1, "s": "one"}}}"#, | ||
| br#"{"payload": {"op": "d", "before": {"id": 2, "s": "two"}}}"#, | ||
| br#"{"payload": {"op": "u", "before": {"id": 3, "s": "old"}, "after": {"id": 3, "s": "new"}}}"#, |
There was a problem hiding this comment.
is the before image always complete?
I thought that some formats may only have partial information
There was a problem hiding this comment.
this only works with primary keys.
| } | ||
|
|
||
| /// Transports that stage input buffers instead of flushing them, e.g., the | ||
| /// fault-tolerant Kafka connector, push the same records into the circuit. |
`Vec<T>::deserialize_with_context_aux` dropped its `aux` argument, so a connector that deserializes a whole batch at a time never passed the record metadata to the records in it. Every columnar ingestion path does that, which left `CONNECTOR_METADATA()` columns NULL for the Delta Lake, Iceberg, and Parquet connectors and for ad-hoc inserts. Thread the metadata through the batch with a seed that calls `deserialize_with_context_aux` on each element. Keeping the existing `DeserializationContext` metadata-free avoids spreading an `AUX: 'de` bound over the tuple, map, and struct implementations that use it. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
2926e6b to
9301197
Compare
|
@mihaibudiu , can you check the compiler change I added? |
An input connector with `soft_delete` set pushes every record it reads to the table as an insertion, including the records its stream deletes, and reports the original polarity of each record in the `is_delete` metadata attribute. The table then accumulates the history of the input stream instead of tracking its current contents, and a column declared as `DEFAULT CAST(CONNECTOR_METADATA()['is_delete'] AS BOOLEAN)` tells the two apart: `true` for a deleted record, NULL for an inserted one. The transformation wraps the input handle of the table rather than each of the stream types in `deinput.rs`, so one wrapper serves every data format as well as integrated connectors, which bypass the parser. `is_delete` metadata is derived once per input message and reused for every deleted record in it. A batch that mixes polarities, which the Delta Lake connector produces in `cdc` mode, is split by polarity, since metadata applies to a whole batch. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
Soft deletes are a property of the connector rather than of the data format, so the tests drive records through each real parser into a table whose `is_delete` column comes from record metadata: JSON `insert_delete`, `debezium`, and `raw`, CSV, raw, Parquet, and Avro `debezium`. Formats that only express insertions are covered too, since they must stay unaffected. Connectors that parse columnar data themselves reach the table through the Arrow stream, so that path is covered directly, including a batch that mixes polarities, and end to end through the Delta Lake connector in `cdc` mode. A controller test covers the property reaching the connector from the pipeline configuration, and two more cover the rejected configurations. CSV records are positional and must therefore carry a value for every column, which overrides the metadata; the test records that behavior. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
The docs include an example query that uses row_number to reconstruct the state of the table. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
Wording, mostly: a table with soft deletes represents the stream of updates it receives rather than accumulating state, so drop "accumulates" from the docs, the changelog, the connector config, the module documentation, and the Python test. Also: * Say that any column can order the changes of a key, with connector metadata as the recommended special case, instead of prescribing a metadata timestamp. * Explain the `is_delete` tie-break as a tie-break, without claiming an arbitrary order. * Report `is_delete = true` in the log line that records a soft-delete connector taking effect. * Note why splitting a mixed-polarity Arrow batch is safe: a table with soft deletes has no primary key, so its updates commute. * Correct the claim that every connector bypassing a parser writes to the Arrow stream; Postgres CDC writes to the record stream. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
…mary key A deletion in a table with a primary key identifies a key rather than a record, so a soft-delete connector has nothing to insert in its place. The runtime already rejects such a configuration; the compiler now rejects it too, and points at the offending property. The same check rejects `soft_delete` on a view connector, since soft deletes describe the records a connector ingests. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
The example materialized the whole change log as a `history` view, which grows without bound. Remove it: the change log is what the table already is, and the `live` view below it is what a reader wants. Add, commented out pending further work, a section explaining that `live` itself needs unbounded state, since a change carrying any timestamp can displace the record that is currently latest for its key, and showing a temporal filter as one way to bound it. The Python test for that variant stays live: it checks that a change older than the window reaches the table yet stays out of the view. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
Four test modules each carried their own copy of the same three functions: a random topic name, a topic creation that waits for the broker, and a best-effort deletion. Collect them in tests/kafka.py, along with a kafka_topics context manager that pairs creation with deletion, and delete the copies. test_connector_status also passed the AdminClient and NewTopic classes around through a _kafka_clients indirection, which the module-level imports already made unnecessary. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
b07b425 to
ef9a921
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
Delta since my prior approval (2926e6b → ef9a921) is polish and a compile-time guardrail — still LGTM.
- Compile-time rejection of
soft_deleteon a table with a primary key and on a view, with source-position-accurate errors and full test coverage (ConnectorTests.testSoftDeleteValidation: happy path, PK rejection, view rejection, and the no-opsoft_delete: falsecase). - Wording pass: "accumulates the history" → "represents the stream of updates / contains the history", propagated through the module doc,
ConnectorConfigdoc, changelog, connectors/index.mdx, and the Python test docstring. The log line at soft-delete takeover now readsis_delete = true, matching the metadata attribute. - Arrow batch split now carries a comment explaining why reordering is safe (no primary key ⇒ updates commute).
- Docs example drops the unbounded
historymaterialized view; a commented-out section documents thatliveitself needs unbounded state and shows a temporal filter as one way to bound it, pending the append-only optimization for soft-delete tables.test_soft_delete_temporal_filter_bounds_statecovers that variant end-to-end (stale record reaches the table but stays out of the view). - Kafka test helpers (
_random_topic,_create_topic,_delete_topic_best_effort) hoisted from four modules intopython/tests/kafka.pywith akafka_topicscontext manager; call sites updated cleanly. Orthogonal to soft deletes but a nice cleanup ride-along.
Fixes #6702
See commit messages.
Describe Manual Test Plan
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes