getReturnTypeImpl accepts one argument, but when that argument matches an existing named collection, hasNamedCollectionArg makes getFirstDataArgIndex return 1. Later executeImpl reads arguments[1] (for the prompt), which is out-of-bounds for a single-argument call.

That means LLMGenerateSQL('existing_collection') can hit undefined behavior instead of returning a clean arity exception. Please add an explicit guard for this ambiguous case (e.g. if first arg resolves to a named collection, require at least 2 arguments).

getReturnTypeImpl accepts one argument, but when that argument matches an existing named collection, hasNamedCollectionArg makes getFirstDataArgIndex return 1. Later executeImpl reads arguments[1] (for the prompt), which is out-of-bounds for a single-argument call.

That means LLMGenerateSQL('existing_collection') can hit undefined behavior instead of returning a clean arity exception. Please add an explicit guard for this ambiguous case (e.g. if first arg resolves to a named collection, require at least 2 arguments).

The function signature/docs expose optional tables and database arguments, but this implementation never reads them: buildSystemPrompt always introspects the catalog and buildUserMessage only uses the first data argument.

This creates a user-visible contract mismatch (queries pass extra arguments that are silently ignored). Please either wire these arguments into prompt construction or remove them from the signature and documentation.

Same arity bug as in LLMGenerateSQL: a single argument is accepted here, but if that argument matches an existing named collection, getFirstDataArgIndex becomes 1 and executeImpl reads arguments[1] for the prompt.

LLMGenerateContent('existing_collection') should fail with a deterministic arity error, not access out-of-bounds memory. Please add an explicit check that when first arg is a named collection, at least one prompt argument is present.

When resp.embeddings is empty, this fallback dereferences resp.embeddings.back(), which is undefined behavior.

If the provider returns HTTP 200 with malformed payload (or no data entries), resp.embeddings.size() is 0, so the first iteration goes to the fallback and can throw an exception or corrupt memory.

Please guard the empty-response case before calling .back() (for example, fail the batch with a clear exception or map all rows in the batch to empty embeddings in or_null mode).

This exception includes the full remote response_body in the query error text. Provider error payloads often echo request details (prompt content and sometimes credential/context details), so this can leak sensitive data into user-visible exceptions and logs.

Please avoid returning the raw body verbatim. A safer pattern is to include only HTTP status + provider error code/message extracted from JSON, or a truncated/sanitized snippet.

When the first argument is not constant, checkAndGetColumn<ColumnConst> returns null and first_arg becomes empty, so the function silently falls back to default_llm_resource instead of using (or rejecting) the provided argument.

That makes calls like generateEmbedding(concat('my_', 'nc'), text, 256) behave incorrectly and unexpectedly. Since endpoint/model selection is query-level, this argument should be required to be a constant string.

Please throw BAD_ARGUMENTS when arguments[0] is not constant in the 3-argument form.

The documented allowed values for llm_on_quota_exceeded here are 'throw' and 'null', but runtime behavior in LLMQuotaTracker::isGracefulQuotaMode also accepts 'break' and 'partial', and quota exceptions explicitly instruct users to set 'break'.

This mismatch makes troubleshooting hard because users get guidance that contradicts the setting docs.

Please align these surfaces (setting docs, validation/accepted values, and exception text) to one canonical set of values.

llm_max_input_tokens_per_query is currently not enforced in practice. checkBeforeDispatch only checks input-token quota when estimated_input_tokens > 0, but all call sites pass 0 (here and in LLMGenerateEmbedding). recordResponse updates input_tokens after the response but never checks the max, so queries can exceed the configured input-token budget without exception.

Please either estimate and pass non-zero input tokens before dispatch, or add an input-token limit check inside recordResponse (with consistent throw/null behavior).

When the provider returns fewer embeddings than requested, this fallback silently reuses the last embedding for all missing rows. That can produce incorrect results by assigning unrelated vectors to inputs without any error.

It is safer to treat size mismatch as an exception (or, for generateEmbeddingOrNull, return empty arrays for affected rows) instead of duplicating resp.embeddings.back().

LLMResultCache::buildKey currently excludes the selected provider/resource identity (e.g. collection name, provider, endpoint).

That means two queries using different named collections can collide in cache when functionName, model, temperature, and prompt text are equal, and then one query can reuse another resource's response. This is incorrect and can leak cross-resource results.

Please include resource identity in the cache key (for example provider + normalized endpoint, or collection identifier) for all LLM text functions.

Same cache-key issue in embeddings: LLMResultCache::buildKey is computed from function name, model, dimensions, and text, but not from selected resource identity (provider/endpoint/collection).

If two named collections use the same model and input text but different endpoints/accounts, cached vectors can be reused across resources. For embedding workloads this is a correctness and isolation issue.

Please include resource identity in the embedding cache key as well.

❌ retry_delay_ms * (1ULL << attempt) can trigger undefined behavior when attempt >= 64 (left-shift past bit width), and the multiplication can overflow before conversion to std::chrono::milliseconds.

Because llm_max_retries is user-configurable and currently unbounded, this path is reachable and can lead to incorrect sleep durations or undefined behavior.

Please clamp the exponent / delay before shifting (or use saturating arithmetic), e.g. cap attempt to a safe max and bound the computed delay to a reasonable upper limit.

❌ extractProviderError is defined with external linkage in both OpenAIProvider.cpp and AnthropicProvider.cpp in namespace DB. This creates duplicate non-inline definitions of the same symbol and can fail at link time (or violate ODR depending on toolchain behavior).

Please move this helper into an anonymous namespace / mark it static, or share a single implementation in one .cpp file and reuse it.

❌ generateEmbedding does not call quota->recordResponse after provider->embed, so embedding requests never contribute to llm_max_input_tokens_per_query / llm_max_output_tokens_per_query enforcement.

As a result, token quotas can be bypassed via embedding functions even when limits are configured. Please record response token usage here (similar to LLMFunctionBase::executeImpl) so quota behavior is consistent across all AI functions.

Typo in this comment: generateEmbeddingOrNull returns Array(Float32), not Nullable(Array(Float32)).

generateEmbedding and generateEmbeddingOrNull share GenerateEmbeddingFunction::getReturnTypeImpl, which returns Array(Float32), and the .reference file also expects Array(Float32).

Please update this comment to avoid confusion.

llm_max_api_calls_per_query is effectively bypassed by retries here.

checkBeforeDispatch increments api_calls once before entering the retry loop, but each retry iteration still sends a new HTTP request via provider->call without any additional quota check. With llm_max_api_calls_per_query = 1 and llm_max_retries = 3, a single row can still emit up to 4 remote calls.

Please account API-call quota per actual HTTP attempt (including retries), e.g. by moving/adding the API-call reservation check inside the retry loop before provider->call.