gengliangwang commented on code in PR #55508: URL: https://github.com/apache/spark/pull/55508#discussion_r3140713642
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveChangelogTable.scala: ########## @@ -0,0 +1,311 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.analysis + +import org.apache.spark.sql.catalyst.expressions._ +import org.apache.spark.sql.catalyst.expressions.aggregate.{Count, Max, Min} +import org.apache.spark.sql.catalyst.plans.logical._ +import org.apache.spark.sql.catalyst.rules.Rule +import org.apache.spark.sql.catalyst.streaming.StreamingRelationV2 +import org.apache.spark.sql.connector.catalog.{Changelog, ChangelogInfo} +import org.apache.spark.sql.errors.QueryCompilationErrors +import org.apache.spark.sql.execution.datasources.v2.{ChangelogTable, DataSourceV2Relation} +import org.apache.spark.sql.types.{IntegerType, StringType} + +/** + * Post-processes a resolved [[ChangelogTable]] read to apply CDC option semantics + * (carry-over removal, update detection) and to enforce supported option combinations. + * + * Fires after [[ResolveRelations]] has wrapped the connector's [[Changelog]] in a + * [[ChangelogTable]]. Both batch ([[DataSourceV2Relation]]) and streaming + * ([[StreamingRelationV2]]) reads are handled: + * - Batch: the requested post-processing passes are injected as logical operators on top + * of the relation. Carry-over removal and update detection are fused into a single + * pass over a (rowId, _commit_version)-partitioned Window: the Filter drops CoW + * carry-over pairs (same rowVersion on both sides) and the subsequent Project relabels + * real delete+insert pairs as update_preimage / update_postimage. + * - Streaming: post-processing is not yet supported. If the requested options would + * require any post-processing, the rule throws an explicit [[AnalysisException]] to + * prevent silent wrong results. Streams that don't require post-processing pass + * through unchanged. + * + * Net change computation (`deduplicationMode = netChanges`) is not yet implemented and + * is rejected up-front for both batch and streaming. + */ +object ResolveChangelogTable extends Rule[LogicalPlan] { + + /** + * Reserved (`__spark_cdc_*`) column names used internally by post-processing; + * connectors must not emit columns with these names. + */ + object HelperColumn { + final val DelCnt = "__spark_cdc_del_cnt" + final val InsCnt = "__spark_cdc_ins_cnt" + final val MinRv = "__spark_cdc_min_rv" + final val MaxRv = "__spark_cdc_max_rv" + final val RvCnt = "__spark_cdc_rv_cnt" + + val all: Set[String] = Set(DelCnt, InsCnt, MinRv, MaxRv, RvCnt) + } + + override def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUp { + case rel @ DataSourceV2Relation(table: ChangelogTable, _, _, _, _, _) if !table.resolved => + val changelog = table.changelog + val req = evaluateRequirements(changelog, table.changelogInfo) + + val resolvedRel = rel.copy(table = table.copy(resolved = true)) + var updatedRel: LogicalPlan = resolvedRel + if (req.requiresCarryOverRemoval || req.requiresUpdateDetection) { + updatedRel = addRowLevelPostProcessing( + resolvedRel, changelog, req.requiresCarryOverRemoval, req.requiresUpdateDetection) + } + if (req.requiresNetChanges) { + updatedRel = injectNetChangeComputation(updatedRel, changelog) + } + updatedRel + + case rel @ StreamingRelationV2(_, _, table: ChangelogTable, _, _, _, _, _, _) + if !table.resolved => + // Streaming CDC reads do not yet apply post-processing. Run the same option / + // capability validation as the batch path so silent wrong results are impossible: + // either no post-processing would be required (fall through, return raw stream), + // or we throw an explicit AnalysisException. + val changelog = table.changelog + val req = evaluateRequirements(changelog, table.changelogInfo) + if (req.needsAny) { + throw QueryCompilationErrors.cdcStreamingPostProcessingNotSupported(changelog.name()) + } + rel.copy(table = table.copy(resolved = true)) + } + + // --------------------------------------------------------------------------- + // Option validation & Requirement Computation + // --------------------------------------------------------------------------- + + /** + * Captures which post-processing passes a CDC query requires, derived from the + * user-provided [[ChangelogInfo]] options and the connector-declared [[Changelog]] + * capability flags. + */ + private case class PostProcessingRequirements( + requiresCarryOverRemoval: Boolean, + requiresUpdateDetection: Boolean, + requiresNetChanges: Boolean) { + def needsAny: Boolean = + requiresCarryOverRemoval || requiresUpdateDetection || requiresNetChanges + } + + /** + * Validates CDC option/capability combinations and computes which post-processing + * passes are required. Throws an [[org.apache.spark.sql.AnalysisException]] for + * unsupported or contradictory combinations (currently: `netChanges` deduplication, + * and `computeUpdates` with surfaced carry-overs but no carry-over removal). + */ + private def evaluateRequirements( + changelog: Changelog, + options: ChangelogInfo): PostProcessingRequirements = { + // Net change computation is not yet implemented. + if (options.deduplicationMode() == ChangelogInfo.DeduplicationMode.NET_CHANGES) { + throw QueryCompilationErrors.cdcNetChangesNotYetSupported(changelog.name()) + } + + val requiresCarryOverRemoval = + options.deduplicationMode() != ChangelogInfo.DeduplicationMode.NONE && + changelog.containsCarryoverRows() + val requiresUpdateDetection = + options.computeUpdates() && changelog.representsUpdateAsDeleteAndInsert() + val requiresNetChanges = + options.deduplicationMode() == ChangelogInfo.DeduplicationMode.NET_CHANGES && + changelog.containsIntermediateChanges() + + // If carry-overs are surfaced and update detection is enabled without carry-over + // removal, carry-overs would be falsely classified as updates, leading to wrong + // results. Hence we throw. + if (requiresUpdateDetection && + changelog.containsCarryoverRows() && + options.deduplicationMode() == ChangelogInfo.DeduplicationMode.NONE) { + throw QueryCompilationErrors.cdcUpdateDetectionRequiresCarryOverRemoval( + changelog.name()) + } + + PostProcessingRequirements( + requiresCarryOverRemoval, requiresUpdateDetection, requiresNetChanges) + } + + // --------------------------------------------------------------------------- + // Row Level Post Processing (Update Detection & Carry-over Removal) + // --------------------------------------------------------------------------- + + /** + * Adds row-level post-processing (carry-over removal and/or update detection) on top of + * the given plan. `counts` = per-partition delete and insert change_type row counts over + * `(rowId, _commit_version)`. `rv bounds` = per-partition min/max of `rowVersion`. + * Equal bounds signal a copy-on-write carry-over. + * - both active -> Window(counts + rv bounds) -> Filter -> Project(relabel) -> Drop helpers Review Comment: Nit: this summary hasn't been updated to reflect the `_rv_cnt = 2` NULL-safety clause. `_min_rv = _max_rv` alone no longer signals a carry-over — both sides must also be non-null. ```suggestion * Adds row-level post-processing (carry-over removal and/or update detection) on top of * the given plan. `counts` = per-partition delete and insert change_type row counts over * `(rowId, _commit_version)`. `rv bounds + rv count` = per-partition min/max of `rowVersion` * plus the non-null count; equal bounds with both sides non-null signal a copy-on-write * carry-over. ``` ########## sql/core/src/test/scala/org/apache/spark/sql/connector/ResolveChangelogTablePostProcessingSuite.scala: ########## @@ -0,0 +1,970 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.connector + +import java.util.Collections + +import org.scalatest.BeforeAndAfterEach + +import org.apache.spark.SparkRuntimeException +import org.apache.spark.sql.{AnalysisException, QueryTest, Row} +import org.apache.spark.sql.catalyst.InternalRow +import org.apache.spark.sql.connector.catalog.{ + ChangelogProperties, Column, Identifier, InMemoryChangelogCatalog} +import org.apache.spark.sql.connector.expressions.Transform +import org.apache.spark.sql.test.SharedSparkSession +import org.apache.spark.sql.types.{ + BinaryType, BooleanType, DoubleType, LongType, StringType, StructField, StructType} +import org.apache.spark.unsafe.types.UTF8String + +/** + * Tests for [[org.apache.spark.sql.catalyst.analysis.ResolveChangelogTable]] using the + * in-memory changelog catalog. These tests don't depend on Delta or any specific connector; + * they directly control what the connector "returns" by populating the in-memory changelog + * with hand-crafted change rows. + * + * Each test sets up [[ChangelogProperties]] on the catalog to enable specific post-processing + * paths (carry-over removal, update detection) and then verifies that Spark's analyzer rule + * correctly transforms the plan and produces the expected output. + */ +class ResolveChangelogTablePostProcessingSuite + extends QueryTest + with SharedSparkSession + with BeforeAndAfterEach { + + private val catalogName = "cdc_test_catalog" + private val testTableName = "events" + + override def beforeAll(): Unit = { + super.beforeAll() + spark.conf.set( + s"spark.sql.catalog.$catalogName", + classOf[InMemoryChangelogCatalog].getName) + } + + override def beforeEach(): Unit = { + super.beforeEach() + val cat = catalog + val ident = Identifier.of(Array.empty, testTableName) + if (cat.tableExists(ident)) cat.dropTable(ident) + cat.clearChangeRows(ident) + cat.setChangelogProperties(ident, ChangelogProperties()) + cat.createTable( + ident, + Array( + Column.create("id", LongType), + Column.create("name", StringType), + Column.create("row_commit_version", LongType, false)), + Array.empty[Transform], + Collections.emptyMap[String, String]()) + } + + private def catalog: InMemoryChangelogCatalog = { + spark.sessionState.catalogManager + .catalog(catalogName) + .asInstanceOf[InMemoryChangelogCatalog] + } + + private def ident = Identifier.of(Array.empty, testTableName) + + /** + * Helper to create a change row matching schema + * (id, name, row_commit_version, _change_type, _commit_version, _commit_timestamp). + * + * `rowCommitVersion` follows Delta row-tracking semantics: carry-over pairs (CoW-rewritten + * unchanged rows) share the same value on both sides; real updates carry the OLD value on + * the delete side and the NEW value on the insert side. Defaults to `commitVersion` for + * tests that don't exercise carry-over removal. + */ + private def changeRow( + id: Long, + name: String, + changeType: String, + commitVersion: Long, + rowCommitVersion: Long = -1L, + commitTimestamp: Long = 0L): InternalRow = { + val rcv = if (rowCommitVersion == -1L) commitVersion else rowCommitVersion + InternalRow( + id, + UTF8String.fromString(name), + rcv, + UTF8String.fromString(changeType), + commitVersion, + commitTimestamp) + } + + // =========================================================================== + // Carry-Over Removal + // =========================================================================== + + test("carry-over removal drops identical delete+insert pairs") { + catalog.setChangelogProperties(ident, ChangelogProperties( + containsCarryoverRows = true, + rowIdNames = Seq("id"), + rowVersionName = Some("row_commit_version"))) + + // v1: insert Alice and Bob (rcv=1 each) + // v2: real delete Alice (preimage carries old rcv=1); + // carry-over for Bob (CoW, rcv unchanged on both sides) + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "insert", 1L, rowCommitVersion = 1L), + changeRow(2L, "Bob", "insert", 1L, rowCommitVersion = 1L), + changeRow(1L, "Alice", "delete", 2L, rowCommitVersion = 1L), + changeRow(2L, "Bob", "delete", 2L, rowCommitVersion = 1L), // carry-over + changeRow(2L, "Bob", "insert", 2L, rowCommitVersion = 1L))) // carry-over (same rcv) + + checkAnswer( + sql( + s"SELECT id, name, _change_type, _commit_version " + + s"FROM $catalogName.$testTableName CHANGES FROM VERSION 1 TO VERSION 2"), + Seq( + Row(1L, "Alice", "insert", 1L), + Row(2L, "Bob", "insert", 1L), + Row(1L, "Alice", "delete", 2L))) + } + + test("deduplicationMode=none keeps all carry-over rows") { + catalog.setChangelogProperties(ident, ChangelogProperties( + containsCarryoverRows = true, + rowIdNames = Seq("id"), + rowVersionName = Some("row_commit_version"))) + + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "insert", 1L, rowCommitVersion = 1L), + changeRow(2L, "Bob", "delete", 2L, rowCommitVersion = 1L), + changeRow(2L, "Bob", "insert", 2L, rowCommitVersion = 1L))) + + val rows = sql( + s"SELECT id FROM $catalogName.$testTableName " + + s"CHANGES FROM VERSION 1 TO VERSION 2 WITH (deduplicationMode = 'none')") + .collect() + + assert(rows.length == 3, "Without dedup, all 3 raw rows should be returned") + } + + test("NULL rowVersion on one side is NOT silently dropped as carry-over") { + // Regression for a NULL-safety hole: min/max skip NULLs, so _min_rv = _max_rv alone + // would match a pair with one NULL and one non-null rowVersion. The _rv_cnt = 2 + // clause in the carry-over filter prevents that. + catalog.setChangelogProperties(ident, ChangelogProperties( + containsCarryoverRows = true, + rowIdNames = Seq("id"), + rowVersionName = Some("row_commit_version"))) + + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "insert", 1L, rowCommitVersion = 1L), + // v2: one side has NULL rowVersion (buggy connector), the other has a real value. + InternalRow(1L, UTF8String.fromString("Alice"), null, + UTF8String.fromString("delete"), 2L, 0L), + changeRow(1L, "Alice", "insert", 2L, rowCommitVersion = 5L))) + + checkAnswer( + sql(s"SELECT id, name, _change_type, _commit_version " + + s"FROM $catalogName.$testTableName CHANGES FROM VERSION 1 TO VERSION 2"), + Seq( + Row(1L, "Alice", "insert", 1L), + Row(1L, "Alice", "delete", 2L), + Row(1L, "Alice", "insert", 2L))) + } + + // =========================================================================== + // Update Detection + // =========================================================================== + + test("update detection relabels delete+insert with different data as update") { + catalog.setChangelogProperties(ident, ChangelogProperties( + containsCarryoverRows = false, // no carry-overs in this test + representsUpdateAsDeleteAndInsert = true, + rowIdNames = Seq("id"), + rowVersionName = Some("row_commit_version"))) + + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "insert", 1L), + // v2: Alice -> Robert (delete old, insert new) + changeRow(1L, "Alice", "delete", 2L), + changeRow(1L, "Robert", "insert", 2L))) + + val rows = sql( + s"SELECT id, name, _change_type, _commit_version " + + s"FROM $catalogName.$testTableName " + + s"CHANGES FROM VERSION 1 TO VERSION 2 WITH (computeUpdates = 'true')") + .orderBy("_commit_version", "_change_type") + .collect() + + val descs = rows.map(r => + s"${r.getLong(0)}:${r.getString(1)}:${r.getString(2)}") + + assert(descs.contains("1:Alice:insert"), s"v1 insert. Got: ${descs.mkString(",")}") + assert(descs.contains("1:Alice:update_preimage")) + assert(descs.contains("1:Robert:update_postimage")) + // No raw delete/insert at v2 + assert(!descs.contains("1:Alice:delete")) + assert(!descs.contains("1:Robert:insert")) + } + + test("delete and insert in different versions are NOT labeled as update") { + catalog.setChangelogProperties(ident, ChangelogProperties( + representsUpdateAsDeleteAndInsert = true, + rowIdNames = Seq("id"), + rowVersionName = Some("row_commit_version"))) + + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "insert", 1L), + changeRow(1L, "Alice", "delete", 2L), + changeRow(1L, "Alice", "insert", 3L))) + + val rows = sql( + s"SELECT _change_type, _commit_version FROM $catalogName.$testTableName " + + s"CHANGES FROM VERSION 1 TO VERSION 3 " + + s"WITH (computeUpdates = 'true', deduplicationMode = 'none')") + .collect() + + assert(!rows.exists(_.getString(0).contains("update_")), + "Delete and insert in different versions should not be labeled as update") + } + + // =========================================================================== + // Composite rowId: partitioning uses every rowId column + // =========================================================================== + // + // With a composite rowId such as Seq("id", "name"), the (rowId, _commit_version) + // window partition must include BOTH columns. A regression that drops one of the + // rowId columns would either falsely merge two different row identities into one + // partition (silently mislabeling unrelated delete/insert pairs as updates) or + // trip the UNEXPECTED_MULTIPLE_CHANGES_PER_ROW_VERSION runtime guard. + + test("update detection with composite rowId keeps different (id, name) tuples raw") { + catalog.setChangelogProperties(ident, ChangelogProperties( + representsUpdateAsDeleteAndInsert = true, + rowIdNames = Seq("id", "name"), + rowVersionName = Some("row_commit_version"))) + + // delete (1, Alice) and insert (1, Bob) at v2. These are DIFFERENT composite + // rowIds; they must NOT be relabeled as update. + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "delete", 2L), + changeRow(1L, "Bob", "insert", 2L))) + + val rows = sql( + s"SELECT id, name, _change_type FROM $catalogName.$testTableName " + + s"CHANGES FROM VERSION 2 TO VERSION 2 WITH (computeUpdates = 'true')") + .collect() + + val descs = rows.map(r => + s"${r.getLong(0)}:${r.getString(1)}:${r.getString(2)}").toSet + + assert(descs == Set("1:Alice:delete", "1:Bob:insert"), + s"Composite rowId must keep different (id, name) tuples raw. Got: $descs") + } + + test("carry-over removal with composite rowId removes pairs per (id, name) tuple") { + catalog.setChangelogProperties(ident, ChangelogProperties( + containsCarryoverRows = true, + rowIdNames = Seq("id", "name"), + rowVersionName = Some("row_commit_version"))) + + // Two independent carry-over pairs at v2, both with id=1 but different names. + // With correct composite-rowId partitioning, each pair lives in its own + // (id, name, _commit_version) partition, has _del_cnt=1 / _ins_cnt=1 and equal + // _min_rv / _max_rv, and gets dropped. With broken (id-only) partitioning, the + // four rows would collapse into one partition with _del_cnt=2 / _ins_cnt=2 and + // the carry-over filter (which requires =1) would keep them all. + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "insert", 1L, rowCommitVersion = 1L), + changeRow(1L, "Bob", "insert", 1L, rowCommitVersion = 1L), + changeRow(1L, "Alice", "delete", 2L, rowCommitVersion = 1L), + changeRow(1L, "Alice", "insert", 2L, rowCommitVersion = 1L), + changeRow(1L, "Bob", "delete", 2L, rowCommitVersion = 1L), + changeRow(1L, "Bob", "insert", 2L, rowCommitVersion = 1L))) + + val rows = sql( + s"SELECT id, name, _change_type, _commit_version " + + s"FROM $catalogName.$testTableName CHANGES FROM VERSION 2 TO VERSION 2") + .collect() + + val descs = rows.map(r => + s"${r.getLong(0)}:${r.getString(1)}:${r.getString(2)}") + assert(rows.isEmpty, + s"Both Alice and Bob carry-over pairs at v2 should be removed. Got: ${descs.mkString(",")}") + } + + // =========================================================================== + // No row identity: post-processing skipped + // =========================================================================== + + test("no capability flags -> post-processing not injected in plan") { + // Default ChangelogProperties has no capability flags set; the rule sees nothing to do. + catalog.addChangeRows(ident, Seq( + changeRow(1L, "Alice", "insert", 1L), + changeRow(2L, "Bob", "delete", 2L), + changeRow(2L, "Bob", "insert", 2L))) + + val df = sql( + s"SELECT * FROM $catalogName.$testTableName " + + s"CHANGES FROM VERSION 1 TO VERSION 2 WITH (computeUpdates = 'true')") + + val plan = df.queryExecution.analyzed.treeString + assert(!plan.contains("__spark_cdc_del_cnt"), + s"Plan must not contain post-processing window helpers. Plan:\n$plan") + assert(!plan.contains("__spark_cdc_ins_cnt"), + s"Plan must not contain post-processing window helpers. Plan:\n$plan") + } + + test("streaming without post-processing options passes through") { + // Streaming reads with no capability flags on the connector and no + // post-processing options must resolve without the rule throwing. + val df = spark.readStream + .option("startingVersion", "1") + .changes(s"$catalogName.$testTableName") + val plan = df.queryExecution.analyzed.treeString + assert(!plan.contains("__spark_cdc_del_cnt"), + s"Streaming plan must not contain post-processing helpers. Plan:\n$plan") + } Review Comment: The new streaming test only covers the pass-through branch (no capability flags). The rejection branch at `ResolveChangelogTable.scala:90–92` — `req.needsAny` → `cdcStreamingPostProcessingNotSupported` — is currently untested. A regression here would silently let a streaming CDC query analyze past the guard and then emit raw change rows without the requested post-processing. Consider adding a companion test that sets `containsCarryoverRows = true` on the connector and asserts the stream read throws `INVALID_CDC_OPTION.STREAMING_POST_PROCESSING_NOT_SUPPORTED`. -- 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]
