jiangxt2 commented on PR #12902: URL: https://github.com/apache/gravitino/pull/12902#issuecomment-5620993892
> Let's agree on the ownership boundary from #12473 first, and on whether writes are in scope for this round. Agreed. Discussion #12473 covered reads; #12846 separately proposed writes. I will prepare a split after we agree on ownership and write scope. One point matters: `spark-connector/spark-common` already authorizes before physical loading in [`BaseCatalog`](https://github.com/apache/gravitino/blob/2f433c36617e0f0dfa5e4aefe219c7b6cb1295ca/spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/catalog/BaseCatalog.java#L264-L295) and [`loadTableForWriting`](https://github.com/apache/gravitino/blob/2f433c36617e0f0dfa5e4aefe219c7b6cb1295ca/spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/catalog/BaseCatalog.java#L606-L630). Generic JDBC permits missing vended credentials and bypass options; applying the Doris policy wholesale would change providers. Shared seams make sense, but strict policy remains provider-specific. ## Contract points 1. **Feature-flag scope.** The session-level flag is intentional. Its semantics differ from Gravitino's `enableIcebergSupport`/`enablePaimonSupport` and Spark's `enableHiveSupport()`, but it follows the same application-level opt-in principle: one application selects a consistent integration mode, while endpoints, credentials, SQL-lane settings, and write policy remain catalog-scoped. This provides a familiar `enableDorisSupport` experience and one contract for all `jdbc-doris` catalogs. Mixed mode is outside the current scope; per-catalog selection can be revisited if maintainers require it. 2. **Doris versions.** Spark/Scala need a binary/API gate. [Doris Connector 26.0.0 supports Doris 1.0+](https://doris.apache.org/docs/4.x/connection-integration/data-integration/spark-doris-connector/), while this PR certifies only 3.0.6.2 and 4.0.6. Probing adds physical access and an allowlist. I propose removing “undefined behavior,” separating official compatibility from Gravitino certification, and rejecting only known-incompatible Doris versions unless maintainers prefer a strict allowlist. 3. **Connector runtime and `doris.write.fields`.** [Connector 26.0.0 derives fields from `StructType`](https://github.com/apache/doris-spark-connector/blob/26.0.0/spark-doris-connector/spark-doris-connector-base/src/main/java/org/apache/doris/spark/client/write/StreamLoadProcessor.java), so the current path is correct after exact schema validation. The adapter should nevertheless set `doris.write.fields` explicitly. The supported runtime remains 26.0.0, but preflight verifies only class presence and its error must not imply version verification. Broader versions require separate compatibility testing. 4. **Public options.** The surface has nine new Gravitino properties for endpoints, write policy, and JDBC SQL-lane configuration, plus nine allowlisted Doris tuning options. Exposing retry, timeout, sizing, and resource controls is reasonable because protected options remain blocked. Gravitino still owns the allowlist, so the groups should be reviewed separately and the tuning set can be reduced. ## Correctness points 1. **Truncate return value.** [Spark `SupportsTruncate`](https://github.com/apache/spark/blob/v3.5.3/sql/catalyst/src/main/java/org/apache/spark/sql/connector/write/SupportsTruncate.java) documents returning “this write builder,” and [Doris 26.0.0 mutates and returns `this`](https://github.com/apache/doris-spark-connector/blob/26.0.0/spark-doris-connector/spark-doris-connector-spark-3-base/src/main/scala/org/apache/doris/spark/write/DorisWriteBuilder.scala); the mock matches. The independent failure IT leaves a pre-populated table empty after truncate and a rejected load; without truncate the baseline row would remain. I will make the other overwrite tests self-contained, but I do not see a current silent-append defect. 2. **Wide decimals.** I agree the end-to-end path is incomplete. Gravitino cannot construct `DecimalType(40,2)`, while the Doris Server converter attempts it instead of falling back to `ExternalType`, unlike ClickHouse. The exact failure may precede the direct-type comparison, but wide-decimal normalization is not established. I will remove the unsupported fallback and revise the corresponding documentation; a later contribution can add the Server conversion and end-to-end coverage together. 3. **Session failure.** Global fail-fast was intentional, but preflight inside the catalog loop permits order-dependent partial registration. If session scope remains, it should run once before registration. The dependency check also verifies class presence, not Connector 26.0.0; enforcement and its message must align. 4. **Docker network.** The fixed subnet and overlap rejection already prevented concurrent Mac fixtures from succeeding. This PR adds the fixed name required by the Doris Compose external-network overlay, but combining it with pre-creation removal introduces a worse race: a second JVM may remove the first JVM's network before attachment. This shared-infrastructure change should not land in this form. Compose should receive a per-run name, plus a per-run subnet if concurrent Mac runs are required. 5. **Column-name case.** The case-sensitive check is intentional and documented as part of the fail-closed exact-schema write contract; it is not inherited from Doris Connector. Reads tolerate case-only metadata differences, while writes require exact field identity before Stream Load. I can relax this only if the agreed write contract permits it and the behavior is verified end to end. 6. **Nullability.** The one-way rule was intentionally added and tested in the adapter; it is not imposed by Doris Connector. However, the documentation requires preserving “nullability direction” without defining it, while the implementation interprets it as one-way compatibility. I agree this should be resolved by enforcing equality or documenting and testing separate read/write rules. For the current fail-closed scope, exact equality is clearer. Could you confirm whether session-level selection without mixed mode is acceptable, whether the first contribution should be read-only, and which strict credential, option, or type seams should be generic? I will update the design and present the split before code changes. What do you think? -- 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]
