inference: add runtime and EMBED_TEXT support - #70139
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds hosted embedding configuration and provider runtime management, introduces the ChangesEmbedding feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SQLExpression
participant Domain
participant EmbedFn
participant Provider
Client->>SQLExpression: Execute EMBED_TEXT(model, text, options)
SQLExpression->>Domain: GetEmbedFn(session context)
Domain-->>SQLExpression: Domain-owned EmbedFn
SQLExpression->>EmbedFn: EmbedWithContext(model, text, options)
EmbedFn->>Provider: Generate embedding
Provider-->>EmbedFn: Vector values
EmbedFn-->>SQLExpression: VectorFloat32
SQLExpression-->>Client: Query result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70139 +/- ##
================================================
- Coverage 76.3253% 75.3368% -0.9885%
================================================
Files 2041 2103 +62
Lines 559518 588170 +28652
================================================
+ Hits 427054 443109 +16055
- Misses 131564 142548 +10984
- Partials 900 2513 +1613
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/set.go (1)
179-193: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAudit plugin
OnGlobalVariableEventstill receives the raw, unredacted secret value.
showValStr(the redacted value) is computed afterplugin.ForeachPlugin(...)runs and is only used for the log line below — the audit-plugin callback at line 182 is passed the unredactedvalStr. Now that this code path handles genuine bearer-token secrets (the six new embedding API-key sysvars), any audit plugin implementingOnGlobalVariableEvent(which typically persists/forwards such events) will capture the plaintext credential.🔒 Proposed fix: redact before the audit callback
+ showValStr := redactGlobalSysVarValueForLog(name, valStr) err = plugin.ForeachPlugin(plugin.Audit, func(p *plugin.Plugin) error { auditPlugin := plugin.DeclareAuditManifest(p.Manifest) if auditPlugin.OnGlobalVariableEvent != nil { - auditPlugin.OnGlobalVariableEvent(context.Background(), e.Ctx().GetSessionVars(), name, valStr) + auditPlugin.OnGlobalVariableEvent(context.Background(), e.Ctx().GetSessionVars(), name, showValStr) } return nil }) - showValStr := redactGlobalSysVarValueForLog(name, valStr) logstr := "set global var"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/set.go` around lines 179 - 193, Compute the redacted value with redactGlobalSysVarValueForLog before invoking plugin.ForeachPlugin, then pass that redacted value to auditPlugin.OnGlobalVariableEvent instead of raw valStr. Reuse the same redacted value for the existing log entry and notifyExternalWorkloadGCLifeTime call.
🧹 Nitpick comments (9)
pkg/inference/sqlembed.go (3)
392-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
io.Writerinstead of the inline interface literal.-func writeEmbeddingCacheKeyPart(writer interface{ Write([]byte) (int, error) }, value []byte) { +func writeEmbeddingCacheKeyPart(writer io.Writer, value []byte) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/sqlembed.go` around lines 392 - 397, Update writeEmbeddingCacheKeyPart to accept io.Writer instead of the inline interface literal, adding the necessary io import and preserving the existing length-prefix and value writes.
140-148: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache cost of 1 per entry makes memory usage dimension-dependent.
With
IgnoreInternalCostand cost1, 10k cached entries can range from a few MB to >100MB depending on model dimensionality (e.g. 3072-dim float32 ≈ 12KB/entry). Consider costing bylen(embedding)with a byte-basedMaxCost, or at least documenting the worst-case footprint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/sqlembed.go` around lines 140 - 148, Update the cache configuration around ristretto.NewCache to account for embedding memory usage rather than assigning every entry a uniform cost of one. Use embedding byte size (based on its dimensionality and element size) for entry costs and configure MaxCost in matching byte units, ensuring the existing embedding cache capacity intent is preserved.
351-354: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove
cache.Wait()outside thee.mucritical section.
e.muis also taken byacquireCall,releaseCall, andClose, so the blockingWait()inrunCallserializes cache flushing behind all embedding calls. Movee.cache.Set/Wait()under the closed check outsidee.mu; Ristretto public methods are safe afterCloseinv0.1.1, and the cache miss only adds one extra provider call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/sqlembed.go` around lines 351 - 354, Update runCall so cache.Set and the blocking cache.Wait occur outside the e.mu critical section, while retaining the err, closed, and call.waiters checks before scheduling the cache update. Do not hold e.mu during cache flushing; rely on Ristretto’s safe public methods after Close as described, and preserve the existing cache-miss behavior.pkg/inference/sqlembed_test.go (2)
281-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win1s timeouts are tight for CI under
-race.
receiveFromChanneland therequire.Eventuallycalls all use a 1-second budget for goroutine handoffs. On a loaded race-enabled runner this is a plausible flake source; 5–10s costs nothing on the happy path.♻️ Suggested timeout bump
- case <-time.After(time.Second): + case <-time.After(10 * time.Second): t.Fatalf("timed out waiting for %s", description)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/sqlembed_test.go` around lines 281 - 291, Increase the timeout budget used by receiveFromChannel and every require.Eventually call in this test file from 1 second to a race-friendly 5–10 second duration. Preserve the existing polling behavior and timeout failure messages.
199-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoop returns on an arbitrary map entry.
for ... { return call.waiters == 2 }inspects only the first iterated entry. There is exactly one in-flight key here so it works, but keying the lookup explicitly (or checkinglen(embedFn.inFlight) == 1 && ...) would be less fragile if the test grows. Same pattern at Lines 247-254.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/sqlembed_test.go` around lines 199 - 206, Update the Eventually assertions around embedFn.inFlight to avoid returning from inside a range over the map; explicitly retrieve the expected in-flight entry or first verify the map contains exactly one entry, then assert that entry’s waiters equals 2. Apply the same change to the matching assertion near the second referenced block.pkg/expression/builtin_inference.go (1)
70-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
github.com/pingcap/errorsfor these new runtime errors.Sibling builtin/vectorized error paths use
errors.Errorf/errors.New, while this new file creates user-facing errors withfmt.Errorfand no pingcap errors import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/builtin_inference.go` around lines 70 - 72, Update the EMBED_TEXT deployment-mode error path in the relevant builtin inference function to use github.com/pingcap/errors.Errorf instead of fmt.Errorf, adding the pingcap errors import and removing the now-unused fmt import if applicable.Source: Coding guidelines
pkg/expression/integration_test/integration_test.go (2)
4583-4606: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate helper functions for toggling deploy mode.
enableStarterDeployModeForEmbeddingTestandenableNonStarterDeployModeForEmbeddingTestare structurally identical apart from the target mode and skip semantics. Consider consolidating into a single parameterized helper to reduce duplication.♻️ Proposed consolidation
-func enableStarterDeployModeForEmbeddingTest(t *testing.T) bool { - t.Helper() - if !kerneltype.IsNextGen() { - return false - } - originalMode := deploymode.Get() - require.NoError(t, deploymode.Set(deploymode.Starter)) - t.Cleanup(func() { - require.NoError(t, deploymode.Set(originalMode)) - }) - return true -} - -func enableNonStarterDeployModeForEmbeddingTest(t *testing.T) { - t.Helper() - if !kerneltype.IsNextGen() { - return - } - originalMode := deploymode.Get() - require.NoError(t, deploymode.Set(deploymode.Premium)) - t.Cleanup(func() { - require.NoError(t, deploymode.Set(originalMode)) - }) -} +func setDeployModeForEmbeddingTest(t *testing.T, mode deploymode.Mode) bool { + t.Helper() + if !kerneltype.IsNextGen() { + return false + } + originalMode := deploymode.Get() + require.NoError(t, deploymode.Set(mode)) + t.Cleanup(func() { + require.NoError(t, deploymode.Set(originalMode)) + }) + return true +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/integration_test/integration_test.go` around lines 4583 - 4606, Consolidate enableStarterDeployModeForEmbeddingTest and enableNonStarterDeployModeForEmbeddingTest into one parameterized helper that accepts the target deploy mode, preserves the NextGen skip behavior, captures the original mode, and restores it during cleanup. Update callers to use the unified helper while retaining the starter helper’s boolean result where required.
4512-4534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.Skip()when starter deploy mode can’t be enabled.When
kerneltype.IsNextGen()is false,enableStarterDeployModeForEmbeddingTestreturnsfalseand the test early-returns, skipping the functionalembed_text, generated-column, and check-constraint assertions while still reporting as passing. Replace the barereturnwitht.Skip("embed_text functional tests require nextgen kernel (starter deploy mode)")so the coverage gap is visible.Also ensure CI runs this package with the
nextgenkernel build tag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/integration_test/integration_test.go` around lines 4512 - 4534, Replace the bare return after enableStarterDeployModeForEmbeddingTest in TestEmbedTextFunction with t.Skip("embed_text functional tests require nextgen kernel (starter deploy mode)") so unsupported environments report the functional test as skipped. Also configure CI to run this package using the nextgen kernel build tag.pkg/config/config.go (1)
1075-1100: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider validating
HostedEmbedding.APIEndpointformat, and confirm whetherAPIKeyPathshould be required.
Valid()only checks thatAPIEndpointis non-empty whenEnabledis true; it never validates that the value is a well-formed (e.g. absolutehttps) URL, unlikeNormalizeOpenAIEmbeddingAPIBaseinpkg/sessionctx/variable/embedding_vars.gowhich enforces scheme/host constraints for the sibling OpenAI base URL. Sincepkg/inferencereportedly usesHostedEmbedding.APIEndpointdirectly as the outbound request base URL, a malformed value (missing scheme, stray whitespace, etc.) would only surface as an opaque runtime request failure instead of a clear startup/config error. Separately,Enabled=truewith an emptyAPIKeyPathis currently accepted byValid()— please confirm this is intentional (e.g. the provider tolerates a missing key file) rather than an oversight mirroring theAPIEndpointrequirement.♻️ Proposed validation addition
if c.HostedEmbedding.Enabled && strings.TrimSpace(c.HostedEmbedding.APIEndpoint) == "" { return fmt.Errorf("hosted-embedding.api-endpoint must be configured when hosted-embedding.enabled is true") } + if c.HostedEmbedding.Enabled { + if u, err := url.Parse(strings.TrimSpace(c.HostedEmbedding.APIEndpoint)); err != nil || !u.IsAbs() || !strings.EqualFold(u.Scheme, "https") { + return fmt.Errorf("hosted-embedding.api-endpoint must be an absolute https URL") + } + }Also applies to: 1616-1618, 1763-1768
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config.go` around lines 1075 - 1100, Update HostedEmbedding.Valid to validate APIEndpoint as a trimmed absolute HTTPS URL with a host, matching the constraints used by NormalizeOpenAIEmbeddingAPIBase, and return a clear configuration error for malformed values. Confirm the provider contract for APIKeyPath; if authentication requires it when Enabled is true, enforce the non-empty requirement in the same validation path, otherwise preserve the intentional optional-key behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/domain/inference.go`:
- Around line 19-38: Make Domain.embedFn an atomic.Pointer[inference.EmbedFn] in
pkg/domain/domain.go:232. Update initInferenceProviders,
closeInferenceProviders, and GetEmbedFn in pkg/domain/inference.go:19-38 to use
Store, Swap, and Load respectively, and close only the value returned by
Swap(nil) so readers do not race with closure or observe an already-closed
pointer.
---
Outside diff comments:
In `@pkg/executor/set.go`:
- Around line 179-193: Compute the redacted value with
redactGlobalSysVarValueForLog before invoking plugin.ForeachPlugin, then pass
that redacted value to auditPlugin.OnGlobalVariableEvent instead of raw valStr.
Reuse the same redacted value for the existing log entry and
notifyExternalWorkloadGCLifeTime call.
---
Nitpick comments:
In `@pkg/config/config.go`:
- Around line 1075-1100: Update HostedEmbedding.Valid to validate APIEndpoint as
a trimmed absolute HTTPS URL with a host, matching the constraints used by
NormalizeOpenAIEmbeddingAPIBase, and return a clear configuration error for
malformed values. Confirm the provider contract for APIKeyPath; if
authentication requires it when Enabled is true, enforce the non-empty
requirement in the same validation path, otherwise preserve the intentional
optional-key behavior.
In `@pkg/expression/builtin_inference.go`:
- Around line 70-72: Update the EMBED_TEXT deployment-mode error path in the
relevant builtin inference function to use github.com/pingcap/errors.Errorf
instead of fmt.Errorf, adding the pingcap errors import and removing the
now-unused fmt import if applicable.
In `@pkg/expression/integration_test/integration_test.go`:
- Around line 4583-4606: Consolidate enableStarterDeployModeForEmbeddingTest and
enableNonStarterDeployModeForEmbeddingTest into one parameterized helper that
accepts the target deploy mode, preserves the NextGen skip behavior, captures
the original mode, and restores it during cleanup. Update callers to use the
unified helper while retaining the starter helper’s boolean result where
required.
- Around line 4512-4534: Replace the bare return after
enableStarterDeployModeForEmbeddingTest in TestEmbedTextFunction with
t.Skip("embed_text functional tests require nextgen kernel (starter deploy
mode)") so unsupported environments report the functional test as skipped. Also
configure CI to run this package using the nextgen kernel build tag.
In `@pkg/inference/sqlembed_test.go`:
- Around line 281-291: Increase the timeout budget used by receiveFromChannel
and every require.Eventually call in this test file from 1 second to a
race-friendly 5–10 second duration. Preserve the existing polling behavior and
timeout failure messages.
- Around line 199-206: Update the Eventually assertions around embedFn.inFlight
to avoid returning from inside a range over the map; explicitly retrieve the
expected in-flight entry or first verify the map contains exactly one entry,
then assert that entry’s waiters equals 2. Apply the same change to the matching
assertion near the second referenced block.
In `@pkg/inference/sqlembed.go`:
- Around line 392-397: Update writeEmbeddingCacheKeyPart to accept io.Writer
instead of the inline interface literal, adding the necessary io import and
preserving the existing length-prefix and value writes.
- Around line 140-148: Update the cache configuration around ristretto.NewCache
to account for embedding memory usage rather than assigning every entry a
uniform cost of one. Use embedding byte size (based on its dimensionality and
element size) for entry costs and configure MaxCost in matching byte units,
ensuring the existing embedding cache capacity intent is preserved.
- Around line 351-354: Update runCall so cache.Set and the blocking cache.Wait
occur outside the e.mu critical section, while retaining the err, closed, and
call.waiters checks before scheduling the cache update. Do not hold e.mu during
cache flushing; rely on Ristretto’s safe public methods after Close as
described, and preserve the existing cache-miss behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51bf0fac-79d5-4356-b27f-78d7974a0cbd
📒 Files selected for processing (35)
.agents/skills/tidb-test-guidelines/references/executor-case-map.md.agents/skills/tidb-test-guidelines/references/expression-case-map.md.agents/skills/tidb-test-guidelines/references/sessionctx-case-map.mdpkg/config/config.gopkg/config/config.toml.nextgen.examplepkg/config/config_test.gopkg/domain/BUILD.bazelpkg/domain/domain.gopkg/domain/inference.gopkg/executor/BUILD.bazelpkg/executor/set.gopkg/executor/set_internal_test.gopkg/expression/BUILD.bazelpkg/expression/builtin.gopkg/expression/builtin_inference.gopkg/expression/builtin_inference_test.gopkg/expression/builtin_threadunsafe_generated.gopkg/expression/function_traits.gopkg/expression/integration_test/BUILD.bazelpkg/expression/integration_test/integration_test.gopkg/expression/scalar_function.gopkg/inference/BUILD.bazelpkg/inference/domainadaptor/BUILD.bazelpkg/inference/domainadaptor/adaptor.gopkg/inference/sqlembed.gopkg/inference/sqlembed_test.gopkg/parser/ast/functions.gopkg/planner/util/null_misc_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/BUILD.bazelpkg/sessionctx/variable/embedding_vars.gopkg/sessionctx/variable/embedding_vars_test.gopkg/sessionctx/variable/sysvar.gopkg/table/constraint.gotests/integrationtest/r/executor/show.result
a44e35d to
49fe12f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/inference/sqlembed.go (2)
187-202: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAPI key file is read on every hosted request.
getHostedEmbeddingAPIKeyis wired asGetAPIKey, so each provider call performs a synchronousos.ReadFileon the request path. Consider caching the contents with a short TTL or mtime check, refreshing only when the file changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/sqlembed.go` around lines 187 - 202, The getHostedEmbeddingAPIKey function synchronously reads the API key file for every hosted embedding request. Add cached key contents with a short TTL or file-mtime validation, returning the cached value while valid and rereading only when the cache expires or the file changes; preserve the existing empty-path and read-error behavior.
351-354: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
cache.Wait()blocks the shared mutex.
e.muserializes everyacquireCall/releaseCallin the Domain, andristretto.Wait()blocks until the internal set buffers drain. Under load this turns cache writes into a global serialization point. Theclosed/waiterssnapshot can be taken under the lock and theSet/Waitdone after unlocking.♻️ Suggested restructure
e.mu.Lock() - if err == nil && !e.closed && call.waiters > 0 && e.cache.Set(key, cloneEmbedding(embedding), 1) { - e.cache.Wait() - } + shouldCache := err == nil && !e.closed && call.waiters > 0 call.embedding = embedding call.err = err if e.inFlight[key] == call { delete(e.inFlight, key) } close(call.done) e.mu.Unlock() + if shouldCache { + e.cache.Set(key, cloneEmbedding(embedding), 1) + } call.cancel()Note this drops the
Wait(); if the test inpkg/inference/sqlembed_test.gorelies on immediate visibility, keep an explicitWait()there instead of on the hot path. Closing the cache concurrently with aSetis safe in ristretto, but confirm against the vendored version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/sqlembed.go` around lines 351 - 354, Move the cache write out of the e.mu critical section in the acquireCall/releaseCall flow: capture the err, closed, and waiters eligibility state while holding e.mu, then perform cache.Set after unlocking without calling cache.Wait on the hot path. Preserve immediate cache visibility only in the relevant sqlembed_test.go test if required, and verify the vendored ristretto behavior for concurrent close and Set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/inference/sqlembed.go`:
- Around line 187-202: The getHostedEmbeddingAPIKey function synchronously reads
the API key file for every hosted embedding request. Add cached key contents
with a short TTL or file-mtime validation, returning the cached value while
valid and rereading only when the cache expires or the file changes; preserve
the existing empty-path and read-error behavior.
- Around line 351-354: Move the cache write out of the e.mu critical section in
the acquireCall/releaseCall flow: capture the err, closed, and waiters
eligibility state while holding e.mu, then perform cache.Set after unlocking
without calling cache.Wait on the hot path. Preserve immediate cache visibility
only in the relevant sqlembed_test.go test if required, and verify the vendored
ristretto behavior for concurrent close and Set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64f26b87-49a3-40e3-bca5-783702fe4563
📒 Files selected for processing (37)
.agents/skills/tidb-test-guidelines/references/domain-case-map.md.agents/skills/tidb-test-guidelines/references/executor-case-map.md.agents/skills/tidb-test-guidelines/references/expression-case-map.md.agents/skills/tidb-test-guidelines/references/sessionctx-case-map.mdpkg/config/config.gopkg/config/config.toml.nextgen.examplepkg/config/config_test.gopkg/domain/BUILD.bazelpkg/domain/domain.gopkg/domain/domain_test.gopkg/domain/inference.gopkg/executor/BUILD.bazelpkg/executor/set.gopkg/executor/set_internal_test.gopkg/expression/BUILD.bazelpkg/expression/builtin.gopkg/expression/builtin_inference.gopkg/expression/builtin_inference_test.gopkg/expression/builtin_threadunsafe_generated.gopkg/expression/function_traits.gopkg/expression/integration_test/BUILD.bazelpkg/expression/integration_test/integration_test.gopkg/expression/scalar_function.gopkg/inference/BUILD.bazelpkg/inference/domainadaptor/BUILD.bazelpkg/inference/domainadaptor/adaptor.gopkg/inference/sqlembed.gopkg/inference/sqlembed_test.gopkg/parser/ast/functions.gopkg/planner/util/null_misc_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/BUILD.bazelpkg/sessionctx/variable/embedding_vars.gopkg/sessionctx/variable/embedding_vars_test.gopkg/sessionctx/variable/sysvar.gopkg/table/constraint.gotests/integrationtest/r/executor/show.result
🚧 Files skipped from review as they are similar to previous changes (23)
- pkg/sessionctx/variable/BUILD.bazel
- pkg/inference/domainadaptor/BUILD.bazel
- pkg/planner/util/null_misc_test.go
- pkg/table/constraint.go
- pkg/domain/BUILD.bazel
- pkg/expression/integration_test/BUILD.bazel
- pkg/inference/BUILD.bazel
- tests/integrationtest/r/executor/show.result
- .agents/skills/tidb-test-guidelines/references/expression-case-map.md
- .agents/skills/tidb-test-guidelines/references/domain-case-map.md
- .agents/skills/tidb-test-guidelines/references/sessionctx-case-map.md
- .agents/skills/tidb-test-guidelines/references/executor-case-map.md
- pkg/parser/ast/functions.go
- pkg/executor/set.go
- pkg/expression/builtin.go
- pkg/domain/domain_test.go
- pkg/expression/scalar_function.go
- pkg/expression/builtin_threadunsafe_generated.go
- pkg/config/config_test.go
- pkg/config/config.go
- pkg/sessionctx/variable/sysvar.go
- pkg/expression/integration_test/integration_test.go
- pkg/expression/BUILD.bazel
|
/retest |
| // TiDBCloudStorageURI used to set a cloud storage uri for ddl add index and import into. | ||
| TiDBCloudStorageURI = "tidb_cloud_storage_uri" | ||
| // TiDBExpEmbedJinaAIAPIKey is the API key to use when calling Jina embedding API. | ||
| TiDBExpEmbedJinaAIAPIKey = "tidb_exp_embed_jina_ai_api_key" |
There was a problem hiding this comment.
what's Exp abbr for?
since they are all for api_key, maybe unify to just one such config with the provider info encoded in the value, such as
tidb_exp_embed_api_key = 'provider=open_ai,key=<the-api-key>'
There was a problem hiding this comment.
Exp stands for experimental; we can add a comment to clarify it.
These variable names also intentionally preserve the existing user-facing SQL configuration interface in tidb-cse. Keeping them aligned avoids requiring users, documentation, deployment scripts, and any configuration automation to maintain different mappings for the two implementations. Since this interface is already used downstream, we would prefer not to rename or restructure it unless there is a clear correctness or major design issue.
There was a problem hiding this comment.
And we prefer provider-specific variables because they preserve standard system-variable semantics: each variable represents a directly readable, writable, resettable, and persistable value.
Using a single variable with provider=...,key=... would make each assignment behave more like a patch command than a normal variable assignment. We would need to additionally define its readback value, reset/default behavior, persistence format, deletion semantics, validation, and escaping rules.
|
@ChangRui-Ryan: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
TiDB already has embedding provider implementations and request batching, but they are not connected to a server-managed runtime or exposed through SQL. Users therefore cannot generate embeddings directly from SQL or share caching, batching, cancellation, and provider configuration across sessions.
This PR adds the runtime and SQL integration required for an end-to-end, explicit embedding workflow in Starter deployments.
What changed and how does it work?
Add Starter-only hosted embedding configuration and dynamic global variables for provider API keys and the OpenAI-compatible API base.
Add a Domain-owned embedding runtime.
Add the experimental
EMBED_TEXT(model, text[, options])SQL function.VECTOR<FLOAT32>and support explicit use inSELECT,INSERT, andUPDATE.NULL, validate JSON object options, ignore reserved@searchoptions, and enforce the vector dimension limit.Automatic embedding materialization,
VEC_EMBED_*_DISTANCE()functions, embedding provenance, and planner/vector-index rewrites are left to a follow-up PR.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Summary by CodeRabbit
embed_textbuiltin for vector embeddings in starter deployments, supporting optional JSON options and provider selection.CHECKconstraints.