924060929 commented on PR #68837:
URL: https://github.com/apache/doris/pull/68837#issuecomment-6136347922
FE architecture feedback on `1aa38c0c543`:
The compatibility fix needs one execution version for both planning and
dispatch. Pinning that version is the right direction, and the current
scan/write/coordinator paths now consume the pinned value. However, I would
like the ownership and compatibility boundaries tightened before approving the
FE design.
1. **Give the execution version a clear owner in the per-execution
context.** The planner already receives `TQueryOptions` before
analysis/optimization. The new implementation extracts the version into
additional constructor arguments, stores it in `ScanContext`, reads it back
through `PlanTranslatorContext.getBeExecVersion()` for write planning, and
copies it into the sink/write handle. Meanwhile, the shorter
`PlanTranslatorContext` and `PluginDrivenTableSink` constructors and the
`ScanContext.Builder` default still read `Config.be_exec_version`. This makes
consistency depend on every caller selecting the correct constructor.
Please establish the version once at the execution/planning entry point
and make the existing planning context expose that snapshot, rather than adding
a separate version argument throughout FE internals. `StatementContext` could
hold or reference the execution snapshot, but its lifecycle must be explicit:
each prepared-statement EXECUTE gets a fresh execution snapshot, while planning
and dispatch for that execution use the same value. Production paths should not
silently fall back to the mutable global config. Keeping a lightweight
immutable `ScanContext` and exposing the version through `ConnectorWriteHandle`
at the connector boundary is reasonable; neither needs to retain the entire
statement context. The issue is the ownership and fallback rules, not copying
an immutable integer by itself.
2. **Keep the complete Hive wire adaptation in one place.**
`PhysicalPlanTranslator.createHiveTableSinkDataPartition()` currently selects
the legacy enum and constructs `direct_hash`/`SKEWED`, while
`DataPartition.toThrift()` recognizes the `direct_hash` string and adds the
legacy `DIRECT_HASH` field. These are two halves of the same compatibility
rule. Please encapsulate the version selection and complete field combination
behind one partition-construction/encoding boundary, so the translator
expresses the required Hive distribution without also assembling half of the
legacy protocol. This does not require removing either BE partitioner or
proving them equivalent in this PR.
Feature-specific admission checks can remain where the necessary information
is available: scan checks after projection/range decisions, connector writer
checks in the connector, and the coordinator consuming the selected version for
dispatch. Moving all such checks into a large shared context would mix
responsibilities again. `CommitDataSerializer` is also an appropriate boundary
for adapting the typed Paimon report into transaction commit data.
These are FE design/maintainability concerns, not a claim of a newly
demonstrated wrong-result or crash bug on this head. Validation here is static
source review; I did not run builds or tests. Please also cover the complete
planning-to-dispatch lifecycle and repeated prepared-statement execution when
changing the snapshot ownership, rather than only testing individual
version-forwarding helpers.
--
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]
