yuqi1129 commented on PR #12902:
URL: https://github.com/apache/gravitino/pull/12902#issuecomment-5618779085
Thanks for the thorough work here, and for opening #12473 before writing
code — the questions you raised there were the right ones.
I'd like us to settle the direction before reviewing this line by line,
because I don't think the current shape is reviewable as one change, and
because one of your own questions is still open.
## The open question from #12473
Your discussion asked two things. The reply answered the first (yes, Doris
support is wanted) but not the second:
> Should this begin as a generic Spark connector extension, a dedicated
Doris adapter, or an external connector with a smaller upstream seam?
That question is still unanswered, and it decides most of this PR's
structure. Your own suggested path also had two steps that we skipped:
> 2. Let maintainers choose the ownership boundary
> 3. Write a design document if the accepted scope crosses modules, then
split implementation and Doris-backed tests into reviewable contributions
That's on us as much as on you. Let's do step 2 now.
Separately, #12473 states "This discussion focuses on governed batch reads",
while this PR adds Stream Load writes, 2PC and truncate overwrite. The write
contract has not been discussed anywhere yet.
## How I'd split it
Reading the four gaps in #12473, they don't all belong in the same place:
| Gap | Where I think it belongs |
| --- | --- |
| Authorization before I/O, credential fail-closed, config rejection and
redaction | **`spark-common`, generic.** This is not Doris-specific. If the
generic JDBC path doesn't guarantee authorization-before-I/O today, that's a
gap for every JDBC provider, and fixing it generically benefits MySQL,
PostgreSQL and OceanBase too. |
| Type normalization for JDBC-lossy types | **A shared type-mapping seam.**
Doris is the first user, not the only one. |
| Tablet reader and native/SQL lane selection | **A Doris adapter.** This is
the genuinely Doris-specific part, and it gets much smaller once the two above
move out. |
| Stream Load writes | **A separate issue and discussion.** Out of the scope
#12473 established. |
The first slice would be small and would improve every JDBC provider. The
Doris-specific slice shrinks to the part that actually needs Doris knowledge.
That also gives each piece a review it can realistically get; 60 files and ~7k
lines in one PR cannot get a responsible review from anyone.
## Contract-level points, independent of the split
1. **The feature flag sits at a different level from everything else it
controls.** `spark.sql.gravitino.enableDorisSupport` is a global Spark session
conf, while `doris-write-mode`, `doris-write-overwrite-mode`, `doris-fenodes`,
`doris-query-port` and the five `doris-jdbc-*` keys are catalog properties.
"Use the specialized adapter" is per-catalog by nature — a session can hold two
`jdbc-doris` catalogs — so I'd expect it to be a catalog property as well.
2. **The version gate fails open in the place it matters most.** Uncertified
Spark or Doris versions "do not automatically fall back; users must leave the
flag disabled". For a design whose selling point is failing closed, this is the
one spot that doesn't: someone running Doris 3.0.5 with the flag on gets
undefined behavior rather than a clear error. I'd rather it refuse to
initialize.
3. **The write contract depends on a third-party implementation detail.**
"an absent `doris.write.fields` is derived by Doris Connector 26.0.0 from the
exact validated Spark input schema" — that is internal behavior of a jar the
user supplies. A version bump can break the contract silently.
4. **The new public API surface is large** — nine `doris-*` catalog
properties plus nine allowed `doris.*` passthrough keys, all of which we have
to keep compatible once released. This is the kind of surface a design doc
normally reviews before it ships.
## Correctness issues found while reading
I reviewed the diff anyway. These stand regardless of how we split, and the
first one is serious.
**1. `INSERT OVERWRITE` can silently degrade into an append.**
`GovernedDorisWriteBuilder35.truncate()` discards the value returned by
`((SupportsTruncate) delegate).truncate()`, and `buildForBatch()` then builds
from the original `delegate`. Spark's `SupportsTruncate` contract permits an
implementation to return a new or derived builder, and Spark itself always
consumes that return value in `V2Writes`. If the official Doris `WriteBuilder`
does that, the truncate is lost and the overwrite becomes a plain Stream Load
append: rows are duplicated instead of replaced, with no error.
`TestGovernedDorisWriteBuilder35` cannot catch this, because it stubs the
assumption it is meant to verify:
```java
when(((SupportsTruncate) delegate).truncate()).thenReturn(delegate);
```
Suggested fix: make `delegate` non-final and assign `delegate =
((SupportsTruncate) delegate).truncate();`, then build from that.
**2. The wide-decimal fallback is unreachable.** `isSafeDecimalFallback`
lets `decimal(40,2)` past the unsupported-type gate, but
`requiresNormalization` never consults it: `baseType("decimal(40,2)")` is
`"decimal"`, which is not in `isAlwaysNormalizedType`, so the column takes the
direct-type branch, where `toSparkType(<logical decimal p=40>)` can never equal
the `StringType` placeholder `buildPhysicalSchema` stored. Reading any table
with a `DECIMAL` of precision above 38 fails with "column type differs", even
though the code intends to normalize it to String. Only the literal
`decimal256` spelling works.
**3. A single catalog's misconfiguration takes down the whole session.** In
`GravitinoDriverPlugin`, `resolveCatalogKind(provider)` is called outside the
`try { registerCatalog(...) } catch (Exception e) { LOG.warn(...) }`, and it
throws when Doris support is enabled on an unsupported build or the Doris jar
is missing from the driver. Every other per-catalog failure degrades to a
warning; this one escapes the loop and aborts `init()`. A user with twenty
catalogs, one of them `jdbc-doris`, who sets the flag but forgets the jar loses
the entire Spark session — and which catalogs were registered first depends on
map iteration order.
**4. The integration-test network rename breaks concurrent runs.**
`ContainerSuite` now sets `cmd.withName(NETWORK_NAME)`, changing the
Testcontainers network from a random UUID to the fixed `gravitino-ci-network`.
Together with the pre-existing "remove `gravitino-ci-network` if it exists"
block, two integration-test JVMs running at once collide: the second either
fails to remove a network that still has attached endpoints, or removes the one
the first JVM's containers are using. The gateway-mode option this change is
for doesn't require the rename.
**5. Write-path column names are compared case-sensitively while the read
path is not.** `DorisWriteSchemaCompatibility35` uses `equals`,
`DorisSchemaCompatibility35` uses `equalsIgnoreCase`. A table whose Gravitino
column is `ID` while Doris reports `id` reads fine and fails every governed
write.
**6. The nullability check rejects the safe direction.** Logical nullable
with physical `NOT NULL` is rejected as "less nullable in Doris". Gravitino
columns default to nullable, so a Doris table with `NOT NULL` columns whose
metadata drifted becomes unreadable, while the genuinely unsafe direction
(logical `NOT NULL`, Doris nullable) is not rejected.
A few things I checked that turned out fine, in case they look odd to other
reviewers: `pruneColumns` overwriting `normalizationRequiresSql` is correct
because `pruneColumns` runs last in `V2ScanRelationPushDown` and
`sqlOperatorSelected` latches; the `pushOffset` limit-minus-offset mirroring
holds for both operator orders; and `DorisCatalogPropertiesMetadata` extending
`JdbcCatalogPropertiesMetadata` keeps the cast in
`JdbcCatalogOperations.initialize` valid.
## What I suggest next
Let's agree on the ownership boundary from #12473 first, and on whether
writes are in scope for this round. Once that's settled, I think the read path
split as above, starting with the generic authorization and credential slice,
gets this in much faster than iterating on the current PR.
Happy to keep reviewing as it lands in pieces.
--
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]
