MarcusKainth commented on PR #4204: URL: https://github.com/apache/iggy/pull/4204#issuecomment-5753168905
Thanks for the depth here. All twelve threads are addressed and the per-thread replies cite the commits. Three things I want on record at the top level. First, the evidence. Rather than argue the before and after, I ran the same test files against a `master` worktree, `d8a8ece`, and this branch. | test | `master` | `d8a8ece` | this branch | |---|---|---|---| | **Fix proof: fails on `master`, passes here** | | | | | `given_a_proto_tagged_run_when_the_container_consumes_should_hand_the_sink_a_proto_payload` | FAIL | PASS | PASS | | `given_an_avro_stream_when_the_sink_consumes_should_receive_json_payloads` | FAIL | PASS | PASS | | `proto_payloads_that_are_not_json_are_stored_as_text` | FAIL | PASS | PASS | | `string_body_proto_payload_is_written_as_text` | FAIL | PASS | PASS | | **Blockers demonstrated: passed on `master`, broke on `d8a8ece`, pass here** | | | | | `given_a_proto_convert_transform_when_the_sink_consumes_should_index_the_document` | PASS | FAIL | PASS | | `given_a_proto_convert_transform_when_the_sink_consumes_should_store_the_rows` | PASS | FAIL | PASS | | `given_proto_text_messages_should_store` | PASS | FAIL | PASS | | **Round-two gaps closed: failed on both earlier refs, pass here** | | | | | `given_auto_payload_proto_holding_json_should_store_queryable_json` | FAIL | FAIL | PASS | | `given_proto_payload_holding_json_should_send_the_document` | FAIL | FAIL | PASS | | `given_proto_text_holding_json_when_extracted_should_preserve_the_document` | FAIL | FAIL | PASS | | `given_proto_text_payloads_holding_json_should_write_them_as_rows` | FAIL | FAIL | PASS | | `given_proto_text_that_is_not_json_should_send_it_as_a_string` | FAIL | FAIL | PASS | | `json_body_pretty_proto_json_is_written_as_one_line` | FAIL | FAIL | PASS | | `json_body_proto_payload_holding_json_is_written` | FAIL | FAIL | PASS | | `proto_payload_holding_json_is_written_as_a_document` | FAIL | FAIL | PASS | | `proto_payloads_holding_json_are_indexed_as_documents` | FAIL | FAIL | PASS | | `row_binary_body_proto_payload_holding_json_writes_bytes` | FAIL | FAIL | PASS | | **Unchanged behaviour: pass everywhere** | | | | | `given_a_json_tagged_run_when_the_container_consumes_should_hand_the_sink_a_json_payload` | PASS | PASS | PASS | | `given_a_raw_tagged_run_when_the_container_consumes_should_hand_the_sink_a_raw_payload` | PASS | PASS | PASS | | `given_a_text_tagged_run_when_the_container_consumes_should_hand_the_sink_a_text_payload` | PASS | PASS | PASS | | `given_auto_payload_proto_text_should_store_text` | PASS | PASS | PASS | | `given_bytes_that_are_not_an_any_when_rebuilt_from_a_wire_tag_should_fall_back_to_raw` | PASS | PASS | PASS | | `given_only_proto_text_that_is_not_json_should_fail_with_invalid_payload_type` | PASS | PASS | PASS | | `given_protobuf_wire_bytes_when_rebuilt_from_a_wire_tag_should_stay_an_any_document` | PASS | PASS | PASS | | `json_body_non_json_proto_payload_is_skipped` | PASS | PASS | PASS | | `proto_text_that_is_not_json_is_written_as_a_string` | PASS | PASS | PASS | | `row_binary_body_non_json_proto_payload_is_skipped` | PASS | PASS | PASS | The rows that fail on `master` and pass here are the fix. Two of them fail on `master` only because it had no `Payload::Proto` arm at all, in Meilisearch and the ClickHouse string passthrough, so they are there for completeness rather than as proof. The middle group is the three blockers demonstrated end to end: ClickHouse, Doris and Elasticsearch each pass on `master`, fail on `d8a8ece`, and pass again now. The rows that pass on all three refs are the streams and paths this PR claims not to touch. Building that matrix surfaced two additions to the Compatibility section. A `text` or `raw` stream with `proto_convert` now hands the sink `Payload::Proto` where `master` handed it `Text` or `Raw`; every sink now treats proto text that is not JSON the way it treats `Text`, including HTTP, which used to base64-encode it. And a descriptor-backed `proto_convert` on a `json` stream was dropped by the SDK on `master`, because the `json` tag sent protobuf bytes through the JSON parser; it now arrives as `Raw`. A second pass on my side after the fixes turned up the same defect in three more sinks and one metric detail, so for completeness: - S3, SurrealDB's auto format and HTTP wrote proto text holding JSON as a string or base64 where `master` wrote the document. All three now read it as the document (`c1bbb4c`); HTTP sends proto text that is not JSON as a plain string, the way it sends `Text`. - `iggy_connector_sink_runs` increments after the calls are made rather than before, so it counts calls that happened (`2dbebc4`). - RowBinary is unchanged on purpose: non-object proto text fails the batch, which is what the same array did on `master` when it arrived as JSON, and whole-batch failure on a bad row is the documented contract. - A run that holds no document now fails in Iceberg where a mixed batch used to skip those messages beside their siblings. They were dropped either way; now they are counted as an error. - No warn log on a split batch. A threshold log would fire on every batch of a descriptor-backed `proto_convert`, and the counter carries the signal. Second, four attributions I would correct. RowBinary never had a failure signal to lose; the builder on `master` is byte-identical and the regression was loss rather than silence. Iceberg is not the same shape as Doris and Delta: it silently drops via `filter_map` and only errors when every row is non-JSON, which is worse, and it has a second site in the dynamic router. Both are fixed. The `unreachable!()` in `flatbuffer_convert.rs` is on `master` at `L172` verbatim; this PR deleted a helper and left the panic where it was. I agree it should be an `Err`, so I have made it one. The SKILL.md item 12 pointer was already stale on `master` as item 10, and I have fixed it as a drive-by. Third, the SDK bump. Agreed and done, 0.4.0 to 0.5.0, with the rebuild requirement in the PR body, and the skill now treats a `Schema` variant changing meaning as breaking. SurrealDB's `iggy_schema` column is covered in its README: it now records the variant received, so avro streams write `json` next to a real document where they previously wrote `avro` next to base64 of JSON text. The http sink comment is updated to per-run wording. The pre-existing items you listed (the encoder per message in `avro_convert`, the dropped serialisation in `proto_convert`, the empty `Payload::Raw`, and the per-Raw clone in Elasticsearch) are all real. The Elasticsearch clone is needed because simd_json mutates its input even on failure; the rest I have left for a follow-up so this PR stays a fix. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
