Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 16902 ms |
TPC-DS: Total hot run time: 82606 ms |
ClickBench: Total hot run time: 14.79 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
09bbecc to
0cf6b9d
Compare
linrrzqqq
changed the title
0cf6b9d to
484656b
Compare
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated code review status: complete after two rounds; requesting changes on exact head 484656bbabce227f611309fa8f58a41efe6f6ee3 for five issues.
Findings: embed-only resources can crash non-EMBED scalar and aggregate consumers; the new embed API key is omitted from statement/audit masking; ALTER discards provider normalization; ALTER can persist a partial dedicated group that the BE selects; and concurrent ALTER/query planning can serialize a partial or mixed group.
Checkpoint conclusions:
- Goal and correctness: dedicated EMBED routing, legacy fallback, and shared tuning-field copying are present, but the five inline findings prevent the feature from being safe across its legal resource lifecycle.
- Scope and simplicity: the nine-file change is focused; no unrelated refactor was found.
- Concurrency: the new multi-key group exposes the existing lock-free
toThriftreader as an actionable partial-snapshot race (inline finding). No separate BE query-context race or lock-order issue was found. - Lifecycle, configuration, performance, and observability: no new static/thread lifecycle, configuration item, material hot-path cost, or metric requirement was introduced.
- Compatibility and FE/BE propagation: both production FE send paths populate the new fields. Legacy resources fall back correctly. The documented BE-first rolling-upgrade direction (old FE to new BE) is safe; the technically unsafe FE-first direction is not a supported topology, so no compatibility comment was filed.
- Parallel and conditional paths: non-EMBED scalar functions and
AI_AGGlack the new group-capability check, and the any-field selector is unsafe when validity checks are skipped; both are inline findings. Text and multimodal EMBED otherwise share the selected resource correctly. - Persistence, transactions, and writes: the property-map image/journal path preserves the new keys and aggregate state bytes are unchanged; no table-data or transaction path is involved. ALTER normalization, structural validation, and snapshot atomicity are covered by the inline findings.
- Tests and results: the added tests cover create-time validation, create-time normalization, proc masking, dedicated selection, shared dimensions, and legacy fallback, but miss the five negative/audit/ALTER/concurrency cases. Per the review prompt, no local build or test suite was run;
git diff --checkpasses. Current CI shows CheckStyle and secret scanning passing, Clang Formatter failing, and build/test jobs skipped.
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 17057 ms |
TPC-DS: Total hot run time: 84058 ms |
ClickBench: Total hot run time: 14.85 s |
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
938e698 to
064a5ba
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Automated review of 064a5ba559ae73b5ec73bd6fc297f916df21e53d: requesting changes for two distinct current-head issues.
The dedicated embedding-property flow is coherent in its basic CREATE, planning, Thrift, and BE selection paths, but the new effort member breaks the existing distributed AI_AGG state format during a supported mixed-BE rollout, and the shared effort allowlist accepts configurations that a selected provider rejects. No additional user focus was supplied, so the full 15-file change was reviewed.
Checkpoint conclusions:
- Goal/tests: basic dedicated-group selection, masking, transport, and request-shape cases are covered; mixed-version aggregate state and provider-negative effort cases are missing.
- Focus/scope: the patch is feature-focused, but it changes a private distributed state format and several provider contracts without their compatibility/validation mechanisms.
- Concurrency: no new distinct issue survived; ALTER/serialization snapshot variants are already covered by existing review threads.
- Lifecycle/static initialization: no static-init or dependency-cycle concern; the aggregate intermediate-state lifecycle exposes the P1 compatibility defect.
- Configuration/dynamic behavior:
ai.effortis persisted resource metadata, not a dynamic server config; its provider/model validation is the P2 issue. - Compatibility/rolling upgrade: M1 is reachable during the documented BE-first mixed-version phase. The raw embed-only Thrift mismatch was checked and dismissed because all BEs are upgraded before any FE under the supported procedure.
- Parallel paths: both fragment builders, scalar AI functions, EMBED, AI_AGG, and OpenAI/Gemini/Anthropic-compatible request builders were traced.
- Conditionals/invariants: dedicated-versus-general fallback and partial groups were checked; surviving partial-group cases are duplicate-fenced by existing threads.
- Testing: FE UT and Cloud UT currently pass, as do formatter/style/license checks. Compile and performance statuses currently fail with authenticated TeamCity details unavailable, and BE UT is pending. No local build or tests were run for this read-only review.
- Observability/errors: provider HTTP errors propagate through the existing status path; no separate logging/metrics defect was found.
- Persistence/transactions/data writes: resource-map persistence and replay were reviewed; no new distinct transaction or table-write issue survived the existing ALTER threads.
- FE/BE variables: the public Thrift fields are updated on both sides, but the separate AI_AGG byte format has no version route.
- Performance: no material hot-path regression or new long-lived allocation was identified.
- Final sweep: two review rounds converged with all Round 2 agents reporting
NO_NEW_VALUABLE_FINDINGS; live threads were refreshed and both comments are non-duplicates.
064a5ba to
f62b987
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes on exact head f62b9872f5a8780e720f914a0ece336f565f0b57. I found five new, non-duplicated issues: one P1 lifecycle race that can reach fatal BE invariants and four P2 correctness/gate issues. Existing review threads were treated as hard duplicate fences.
Checkpoint conclusions:
- Goal and proof: dedicated embed properties, fallback selection, general-consumer guards, key masking, effort transport, and version gating are present; direct unit tests cover the main happy paths, but the five inline findings prevent approval.
- Scope/focus: all 20 authoritative changed-file entries match the diff. The embed work is localized, while the bundled effort field and exec-version bump expand the compatibility surface. No additional review focus was supplied.
- Concurrency/lifecycle:
AIResource.propertiesand the global resource-name map are shared across sessions. Mutation is locked, but validate/install and analysis/transport are not atomic; two inline findings cover those distinct gaps. Per-query BE copies and adapter ownership otherwise introduce no new leak or static-init issue. - Configuration/compatibility: no new dynamic server setting is added. FE and BE maximum exec versions both move from 14 to 15. Optional Thrift fields and the version-15
AI_AGGstate format are internally matched for the supported BE-first rollout, and inspected aggregate paths propagate the negotiated version. The existing same-field wire-format thread is not repeated. - Parallel paths/conditions: both FE transport builders, explicit/default resources, both legality phases, scalar AI,
AI_AGG,EMBED, dedicated/fallback selection, and normal/bucketed/streaming/spill/analytic/nullable aggregate paths were traced. Dedicated provider validation still lacks an embedding-capability contract. - Tests/results: test assertions and oracles were inspected. Exact-head CheckStyle and BE header hygiene pass. Exact-head Clang Formatter fails, reproduced non-mutatingly at the inline test line. No build or unit suite was run under the review-only prompt. Missing coverage includes the accepted concurrent ALTER, same-name replacement, unsupported embed provider, and effort-clear transitions.
- Observability/persistence/writes: existing provider status/logging is adequate. Whole-resource Gson logging/replay carries the new keys, but atomic metadata mutation and optional-property clearing are defective as commented. No table-data write path changes.
- FE-BE state/performance: both FE senders, generated Thrift presence,
QueryContextinstallation, BE conversion, adapter configuration, and aggregate serialization were checked. No material hot-path CPU, allocation, or network-amplification regression was found. - Other: dedicated API-key masking is fixed on this head; no additional distinct issue survived three convergence rounds.
f62b987 to
83a2ac7
Compare
|
run buildall |
TPC-H: Total hot run time: 17135 ms |
TPC-DS: Total hot run time: 82809 ms |
ClickBench: Total hot run time: 14.8 s |
|
Reviewed commit
For findings 2–4, the EMBED path reaches From a reuse and scope perspective, I would keep the single configuration-selection hook and make CREATE/ALTER share the validated snapshot logic. I would also split Validation: the existing |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
The above comments are not caused by this pr, and the subsequent fix of |
|
One remaining [P2] issue outside the five resource validation/consistency findings in my earlier comment: validate On the current head ( References: This affects requests that explicitly configure an unsupported effort value; existing usage that omits effort is unaffected by this issue. The expected upstream rejection follows from the code and API contract; I did not reproduce it against a live provider endpoint. This was previously reported by the automated reviewer, so it is not a newly discovered finding. The current diff of #68023 does not cover effort validation. Please include provider-specific effort validation and negative tests in the follow-up, or separate the effort feature from this PR. Apart from the previously discussed resource issues and this P2, this re-review did not identify an additional P1 blocker. If these known issues are accepted for follow-up, I have no additional code-level objection to merging this PR first. This is not a claim that the outstanding issues are fixed. Validation for this re-review: changed-file clang-format 16.0.6, BE build hygiene, and diff whitespace checks passed. Builds and runtime tests were not rerun. |
|
/review |
There was a problem hiding this comment.
Requested changes for one new P2 correctness issue: AI_AGG silently applies different request semantics at supported execution version 14 depending on whether the optimizer chooses a one- or two-phase plan. The inline comment contains the trace and remediation options.
Critical checkpoint conclusions:
- Goal and proof: dedicated embedding properties are wired through FE validation/transport and BE selection, with focused unit coverage. The bundled
ai.effortaddition is not correct for every supportedAI_AGGplan because of the inline issue. - Scope/focus: the PR is broader than its stated embedding goal because it also adds effort handling and an aggregate-state version. That compatibility-sensitive addition is where the new defect lies.
- Concurrency/thread safety: resource mutation, snapshot, and identity paths were traced. Remaining concerns are already covered by existing review threads; no distinct deadlock, lock-order, or new race finding was added.
- Lifecycle/static initialization: no new non-intuitive ownership, release, cross-TU initialization-order, or static-lifecycle defect was found.
- Configuration:
be_exec_versionis mutable and version 14 remains supported, so silently dropping a configured property only in serialized plans is not a safe dynamic-configuration behavior. - Compatibility: version propagation is consistent across operators and wrappers, and the new guard bounds legacy rows, but version 14 now has plan-dependent effort semantics. This is distinct from the existing legacy-state overread thread.
- Parallel paths: both FE resource transports, scalar AI, embedding, aggregate, regular/streaming/bucketed/analytic aggregation, and relevant wrappers were checked. No additional omitted path survived duplicate fencing.
- Conditional checks/error handling: general/embed group selection and provider capability paths were checked; remaining defects in those checks are already covered by existing comments.
- Tests: changed unit tests cover resource masking/normalization, payloads, and versioned state bytes, but no distributed version-14 test catches the one-phase/two-phase semantic split.
- Test results: review was static-only as required; no builds or tests were run. Current CheckStyle and Clang Formatter CI checks pass, the local text-only build-hygiene check passed, and substantive build/test jobs shown for this head are skipped.
- Observability: existing error propagation is adequate for the changed paths; no separate logging/metrics gap was substantiated.
- Persistence/transactions/data writes: resource persistence/replay was traced and remaining concerns are already-commented duplicates. This PR does not introduce a database transaction or data-write path requiring another finding.
- FE-BE variables/protocol: all identified resource senders and Thrift consumers were checked. Optional-field and BE-first rolling-upgrade paths produced no additional distinct issue.
- Performance: no material CPU, memory, allocation, or complexity regression was substantiated.
- Other: the PR body says
Release note: Nonedespite user-visible resource properties and behavior; please reconsider the release-note/documentation status.
User focus: no additional focus was provided. The full 20-file authoritative diff was reviewed.
| buf.write_binary(_task); | ||
|
|
||
| _ai_config.serialize(buf); | ||
| _ai_config.serialize(buf, be_exec_version >= SUPPORT_AI_AGG_EFFORT_VERSION); |
There was a problem hiding this comment.
[P2] Do not silently discard effort in version-14 partial states. Version 14 is still a supported mutable be_exec_version, and AI_AGG supports two-phase plans. In a LOCAL/partial stage, prepare() loads the configured effort, but this serialization omits it; the GLOBAL stage then rebuilds the adapter with an empty effort and sends a different request. The equivalent one-phase plan never serializes the state and keeps the effort, so behavior depends on plan shape. Please either reject AI_AGG resources with effort when be_exec_version < 15, or preserve the setting outside the legacy state format, and cover the distributed v14 path.
5127b2d to
4b77569
Compare
4b77569 to
0806066
Compare
|
/review |
There was a problem hiding this comment.
Static review of exact head 080606663f8f4c8f9f6a8f3d5a06ebe38ea27766 against base af525d567d5352fb8ee2bead6aab3dbc9b23984c found no new valuable issue distinct from the existing inline review threads. This is a summary-only COMMENT review; it does not resolve or duplicate those threads.
Critical checkpoint conclusions:
- Goal and correctness: dedicated
ai.embed.*selection and general fallback, API-key masking, andai.effortpropagation were traced across FE validation and persistence, both FE transport builders, Thrift, BE resource selection, provider payload construction, andAI_AGGstate handling. No new correctness issue survived the duplicate fence. - Scope and user focus: the authoritative 17-file diff exactly matches the supplied changed-file list. The change also carries effort support beyond the title's embedding-property focus. No additional user-provided focus was supplied.
- Concurrency and lifecycle: whole-map publication avoids entry-by-entry mixing, but the already-raised pre-lock concurrent ALTER, lock-free/name-only transport, and same-name DROP/CREATE identity concerns remain covered by existing threads. No distinct BE ownership, static-lifecycle, or lock-order issue was found.
- Configuration, compatibility, persistence, and writes: legacy Thrift IDs are retained and the supported BE-first upgrade direction remains readable. The optional row-framed trailing effort field is bounded by each
AI_AGGstate row and supports old-writer/new-reader handling at this head. Resource keys persist through snapshot/journal paths. No database transaction or table-data write path is changed. - Parallel and conditional paths: general-only, embed-only, combined, partial-group, scalar, aggregate, text-embedding, multimodal-embedding, one-phase, and two-phase paths were checked. Remaining partial-group, provider-capability, value-domain, resource-identity, and version-specific concerns are already represented by existing threads.
- Error handling, security, performance, and observability: provider status propagation, nullable/COW behavior, audit/SHOW secret masking, and relevant invariants were inspected. No new silent error loss, in-scope credential disclosure, material hot-path regression, or observability gap was substantiated.
- Tests: added FE and BE unit tests cover group validation, masking, persistence, provider payloads, state serialization, and dedicated-versus-fallback selection. Missing concurrency, unsupported-provider, and mixed-version coverage is already requested in existing threads. Per the review instructions, this review was static-only; no builds or tests were run.
All three bounded review tracks returned NO_NEW_VALUABLE_FINDINGS, and the final changed-file, unresolved-candidate, live-head, and live-comment sweeps were clean.
|
run buildall |
TPC-H: Total hot run time: 28578 ms |
TPC-DS: Total hot run time: 154922 ms |
ClickBench: Total hot run time: 24.17 s |
FE UT Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
ai.embed.endpointai.embed.provider_typeai.embed.model_nameai.embed.api_keyai.*group, orai.embed.*groupai.embed.*when executing theEMBEDfunction if the embed property group is configured.ai.*properties otherwise.ai.embed.api_keywhen displaying resource properties.ai.*properties.Release note
None