fix(vdb-tidb-vector): swap FTS_MATCH_WORD argument order to match TiDB - #41836
Open
Taranum01 wants to merge 8 commits into
Open
fix(vdb-tidb-vector): swap FTS_MATCH_WORD argument order to match TiDB#41836Taranum01 wants to merge 8 commits into
Taranum01 wants to merge 8 commits into
Conversation
…enius#41594) `sqlalchemy.engine` (and its sub-loggers like `sqlalchemy.pool`) have `propagate = False` set in ext_logging.init_app to avoid duplicate logs, so the root logger's LOG_TZ-aware formatter never gets a chance to format SQLAlchemy records. When SQLALCHEMY_ECHO is enabled, engine log timestamps are emitted in the server's local timezone while the surrounding application logs are already converted to LOG_TZ, producing inconsistent timestamps within a single log stream. This change adds `apply_timezone_to_sqlalchemy_loggers()` in ext_logging.py and calls it from ext_database.init_app() right after the SQLAlchemy engine is eagerly created. The helper walks the SQLAlchemy logger hierarchy and applies the same LOG_TZ converter to the formatters of any handlers SQLAlchemy has attached, so engine log timestamps agree with the rest of the application logs. The fix preserves the existing non-propagating behavior (no duplicate logs) and is a no-op when LOG_OUTPUT_FORMAT is not "text" or when LOG_TZ is unset. Regression test covers: - JSON output format: no-op - LOG_TZ unset: no-op - LOG_TZ set: converter patched on sqlalchemy.engine, .pool, and bare SQLAlchemy loggers - Handler with no formatter: left alone, no exception - Idempotency across repeated calls
…logging The langgenius#41594 / langgenius#41629 fix (langgenius#41629) called apply_timezone_to_sqlalchemy_loggers() from ext_database.init_app(), which adds a new import edge extensions.ext_database -> extensions.ext_logging. Combined with the pre-existing libs.oauth_bearer -> extensions.ext_database -> extensions.ext_logging -> core.logging.* chain, importlinter's backend-layers contract now reports libs transitively reaching core. Add the new edge to the ignore_imports list, alongside the existing extensions exceptions (libs.external_api -> extensions.ext_logging, etc.). Siblings inside the extensions package are free to depend on each other; this exception is purely to mark the new edge and keep the migration baseline honest.
…lper (langgenius#41594 / langgenius#41629 follow-up) - Add `-> None` to every test method. - Add a small `_formatter_converter(formatter)` helper that asserts the formatter is not None and casts the converter to a typed `Callable[[float], time.struct_time]`, then route every `formatter.converter` access through it. - All 6 tests still pass; pyrefly reports 0 diagnostics on the file.
…anggenius#41649) The PUT/DELETE/auth endpoints for MCP providers accept a ``provider_id`` in the request body and pass it straight to SQLAlchemy, which (via the ``StringUUID`` column type) sends it to Postgres as a UUID. When a non-UUID value (e.g. ``"fast-mcp"``) is sent — typically when a stale server identifier ends up in the request body — Postgres raises ``psycopg2.errors.InvalidTextRepresentation: invalid input syntax for type uuid`` and the user sees a generic 500. The API layer should fail earlier with a clean 400 instead. This change adds a ``_validate_uuid`` helper and a ``field_validator`` on each of the three payloads that take ``provider_id`` (``MCPProviderUpdatePayload``, ``MCPProviderDeletePayload``, ``MCPAuthPayload``). The helper accepts the existing UUID string form (``xxxxxxxx-xxxx-...``) and rejects anything else with a Pydantic ``ValidationError`` that the existing ``@model_validate`` decorator turns into a 400. Tests (7 new, in ``api/tests/unit_tests/controllers/console/workspace/test_mcp_provider_id_uuid.py``): - ``MCPProviderUpdatePayload`` accepts a valid UUID, rejects ``"fast-mcp"`` and ``""`` with a ValidationError on the ``provider_id`` field. - ``MCPProviderDeletePayload`` and ``MCPAuthPayload`` accept a valid UUID and reject a non-UUID string. All 32 pre-existing tests in ``test_tool_providers.py`` still pass. ruff check / ruff format --check clean, pyrefly 0 diagnostics, importlinter 25 kept / 0 broken.
…tests (langgenius#41649 follow-up) mypy + pyrefly flag every test method in the new test_mcp_provider_id_uuid.py as missing a return annotation. Add `-> None` to all seven test methods. Pure test annotation, no behavior change.
…langgenius#41714) `_get_uuids` overrode the base ``VectorBase._get_uuids`` with ``uuid5(URL_NAMESPACE, page_content)`` (UUID v5) so the same content inserted twice would dedupe to a single Weaviate object. That was self-consistent, but the cleanup path (``batch_clean_document_task`` → ``index_processor.clean`` → ``vector.delete_by_ids``) was passing the segment's ``index_node_id`` from the database — a random UUID v4. ``uuid5(content)`` never equals ``index_node_id``, so ``delete_by_id`` silently no-op'd on every Weaviate vector store and ``Cleaned documents ...`` was logged as success while the objects remained as searchable orphans. Use the same source the rest of the VDB stack uses (``VectorBase._get_uuids``): the ``doc_id`` stored in each document's metadata. Insert writes under that id, the cleanup path already passes the same id, and ``delete_by_ids`` now actually targets the object that was inserted. The deduplication property previously claimed by the UUID5 path is already provided by the upstream ``_filter_duplicate_texts`` step on the same ``doc_id``, so removing the UUID5 hash does not change semantics for any caller that already provides a stable ``doc_id``. The pre-fix UUID5 path is removed; the ``_get_uuids`` override now matches the parent's parameter name (``texts``) so pyrefly stops flagging the override as inconsistent. Tests (2 new in ``providers/vdb/vdb-weaviate/tests/unit_tests/test_weaviate_vector.py``): - ``test_get_uuids_uses_doc_id_to_match_cleanup_path`` — pins the contract: any document with a ``doc_id`` must round-trip through ``_get_uuids`` as that exact ``doc_id``, so insert and delete agree on the Weaviate object id. - ``test_get_uuids_skips_documents_without_doc_id`` — documents without a ``doc_id`` are skipped, so ``add_texts`` falls back to a fresh uuid4 for them (inherited from the base class). All 41 existing tests in the suite still pass.
Contributor
Pyrefly Type Coverage
|
langgenius#41822) TiDB's ``FTS_MATCH_WORD`` signature is ``FTS_MATCH_WORD(query, column)``, but the pre-fix code generated ``FTS_MATCH_WORD(text, :query)`` (column first, query second). TiDB rejected the bound-parameter form with ``This version of TiDB doesn't yet support 'match against a non-constant string'``, silently breaking Full-Text Search and Hybrid Search for every TiDB Vector user with ``TIDB_VECTOR_ENABLE_FULLTEXT_SEARCH=true``. The two occurrences of the function call in ``search_by_full_text`` — the WHERE clause predicate and the score projection — both need the query as the first argument and the column as the second. Tests: - The existing ``test_search_by_full_text_queries_tidb_fts_and_scores`` assertion is updated to the new argument order. - A new ``test_search_by_full_text_uses_query_first_argument_order`` pins the contract: both occurrences in the generated SQL must use ``FTS_MATCH_WORD(:query, text)`` (and never ``FTS_MATCH_WORD(text, :query)``), so the swap can't regress silently. All 29 existing tests still pass. ``ruff check`` / ``ruff format --check`` clean, ``importlinter`` 25 kept / 0 broken.
Taranum01
force-pushed
the
fix/41822-tidb-fts-argument-order
branch
from
September 4, 2026 20:28
2073fc0 to
d879726
Compare
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #41822
What problem does this PR solve?
TiDB's
FTS_MATCH_WORDsignature isFTS_MATCH_WORD(query, column), but the pre-fix code generatedFTS_MATCH_WORD(text, :query)(column first, query second). TiDB rejected the bound-parameter form withThis version of TiDB doesn't yet support 'match against a non-constant string', silently breaking Full-Text Search and Hybrid Search for every TiDB Vector user withTIDB_VECTOR_ENABLE_FULLTEXT_SEARCH=true.What is changed and how it works?
The two occurrences of the function call in
search_by_full_text— the WHERE clause predicate and the score projection — both need the query as the first argument and the column as the second:FTS_MATCH_WORD(:query, text)How it was tested?
test_search_by_full_text_queries_tidb_fts_and_scoresassertion is updated to the new argument order.test_search_by_full_text_uses_query_first_argument_orderpins the contract: both occurrences in the generated SQL must useFTS_MATCH_WORD(:query, text)(and neverFTS_MATCH_WORD(text, :query)), so the swap can't regress silently.