naveenp2708 commented on PR #57644: URL: https://github.com/apache/spark/pull/57644#issuecomment-5320858858
> Thanks for the iterations here -- accepting the declaration and then materializing the engine-owned shape is the right split, and using the flow-effective resolver in both paths keeps this consistent with the rest of AUTO CDC. > > Inline comments below. Two things that seemed better here than inline: > > * **What should a downstream consumer see?** I'd recommend giving `VirtualTableInput.load` the same schema `materializeTable` produces (strip the declared reserved fields, append the flow-inferred ones) through one shared helper, rather than returning the declaration verbatim. Nothing projects the reserved column away today -- the external reads are bare `reader.table(...)` calls, and the cross-pipeline test in `AutoCdcScd1MultiPipelineSuite` drops it by hand with an explicit `.select(...)` -- so the contract is "visible on the target, project it away yourself". This PR makes it hidden at analysis time yet present at execution time and in the table, which is neither. > Step by step, with a data-only declaration on the target and an ordinary downstream consumer: > ```sql > CREATE STREAMING TABLE target (id INT, name STRING, version BIGINT); > CREATE FLOW writer AS AUTO CDC INTO target FROM STREAM(source) KEYS (id) SEQUENCE BY version; > CREATE MATERIALIZED VIEW enriched AS SELECT * FROM target; > ``` > > > > > > > > > > > > > 1. **Planning.** `enriched` resolves `target` through `VirtualTableInput`, which hands back the declaration as-is: 3 columns. > 2. **Materialization.** `target` is created with 4 columns, since this PR appends `__spark_autocdc_metadata`. `enriched` is created with the 3 columns from step 1. > 3. **Execution.** `enriched` is re-analyzed; `target` falls outside the subgraph and is read from the catalog instead, so `SELECT *` now expands to 4 columns. > 4. **Result.** 4 columns written into the 3-column `enriched`. > > Actually hiding the column from consumers instead would need a projection on the read path -- a larger change I wouldn't fold in here. > * **Incremental coverage.** Both new materialization tests create the target from scratch. Is a case where it already exists and goes through `evolveTable` with `mergeWithExistingSchema = true` worth adding, or do you consider that covered by the merge semantics? / -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
