docs: correct the SEARCH table function reference by jackylee-ch · Pull Request #786 · lance-format/lance-spark

jackylee-ch · 2026-08-26T07:15:09Z

Summary

search_columns is required with no positional slot, so SEARCH needs named args (Spark 3.5+)
Drop the positional example, which always throws, and offset, which is never read
Fix Execution (scan path is namespace-dependent) and clarify the current test markers

Test plan

mkdocs build --strict

🤖 Generated with Claude Code

@disabled

search_columns is required (LanceSearchTableFunctions.search throws when it is empty) but has no positional slot, so the documented positional example always fails and SEARCH in fact requires named arguments. offset is documented but never read by search(); unknown named arguments are silently ignored, so it looks accepted. Execution described the removed bespoke single-partition path; the scan now runs server-side through queryTable only when the namespace supports it, and per-fragment otherwise. Validation claimed Docker coverage while the pytest case is xfail and the JVM case is @disabled.

geruh · 2026-09-02T15:25:55Z

 ## Execution

-Spark plans `SEARCH` as a DataSource V2 batch read with one input partition. The partition reader calls the Lance namespace `queryTable` API. With a directory namespace the search runs in the Spark process executing that reader; with a REST namespace the REST server handles the namespace request.
+Spark plans `SEARCH` as a batch read carrying the full-text query as a scan option, wrapped in an optional filter, a projection, `ORDER BY _score DESC`, and `LIMIT k`. The scan then runs one of two ways: a single-partition server-side read through the Lance namespace `queryTable` API when the namespace supports it, or a distributed per-fragment scan for catalog-only namespaces and for reads that target a branch or tag.


I haven't see how search can target a branch or tag is this true or should we drop?

Tag is not reachable, you are right to push on it: version goes through optionalLong, so a tag name never parses, and there is no tag_ identifier suffix to carry one. Branch is reachable, since resolveLanceTable calls loadTable and that matches BRANCH_SUFFIX, so table => '...docs.branch_audit' lands on the branch and shouldNamespaceFtsScan falls back, but nothing tests that path. Dropped the clause in 52baafa and left only the catalog-only condition. Happy to put branch back if you want it documented.

Two corrections to my last reply. I over-corrected by dropping branch as well as tag, and nothing tests that path was wrong — LanceScanBuilderTest#testBranchFullTextQueryDoesNotUseNamespaceScan (LanceScanBuilderTest.java:374) already asserts shouldNamespaceFtsScan() is false for LanceRef.ofBranch. 4ea4cb3 makes the per-fragment scan the general fallback and lists the three conditions the server-side route needs.

…erence

Review feedback: a tag cannot reach SEARCH at all. The version argument goes through optionalLong, so a tag name never parses, and there is no tag_ identifier suffix to carry one. A branch is reachable, since resolveLanceTable calls loadTable and that matches BRANCH_SUFFIX on the identifier, but nothing tests it. Leave the condition users can act on and drop the rest.

I over-corrected: only the tag half of the previous clause was wrong, and dropping branch too made the routing read as if catalog-only namespaces were the sole fallback trigger. State the fallback as the otherwise case and list the three conditions the server-side route needs, matching shouldNamespaceFtsScan: a namespace that implements queryTable, a ref that is not a branch or tag, and no pushed aggregation. Enumerating only the fallback triggers went stale twice.

jackylee-ch · 2026-09-15T07:30:22Z

cc @LuciferYang

LuciferYang

Thanks for the cleanup — the corrections all match the code: search_columns is required with no positional slot, offset is never read on the SEARCH path, and the two-route Execution rewrite (server-side queryTable vs per-fragment scan) lines up with LanceScanBuilder. Two documentation notes below, both non-blocking.

LuciferYang · 2026-09-15T14:09:19Z

 ## Validation

-The Docker integration suite covers `SEARCH` against the directory namespace and a REST namespace backed by a directory namespace. The `Spark Search Docker` GitHub Actions workflow runs both backends for pull requests.
+The `Spark Search Docker` workflow exercises `SEARCH` against directory and REST-directory namespaces. The Docker test still carries an `xfail` marker, but it can pass as `XPASS`; the JVM `SEARCH` cases remain `@Disabled`. [`VECTOR_SEARCH`](vector-search.md) and [`HYBRID_SEARCH`](hybrid-search.md) are also exercised by the workflow.


The Validation section leans on test-framework jargon (xfail, XPASS, @Disabled) that means little to someone reading the feature reference. More importantly, the reason those markers exist is that SEARCH's server-side queryTable route currently ignores the structured FTS query and cannot produce the _score column that the plan always projects and sorts by, so the query fails on the _score projection (the Python case is xfail with raises=Exception). That is exactly the path the Basic Usage example above takes on a queryTable-capable namespace such as a directory namespace, where it selects _score. So the section reads like "tested and passing" while the feature's headline example is actually broken.

Consider collapsing the CI mechanics into a one-line coverage statement and adding a short known-limitation note near the top: on a queryTable-capable namespace SEARCH currently fails on the _score projection. Leave the xfail/@Disabled details in the test code.

LuciferYang · 2026-09-15T14:09:22Z

-!!! note "Named Arguments"
-    Named arguments require Spark 3.5 or later. On Spark 3.4, use the positional form.
+!!! note "Named Arguments Required"
+    `search_columns` is required and has no positional slot, so `SEARCH` must be called with named arguments. Named arguments require Spark 3.5 or later, so `SEARCH` is not available on Spark 3.4.


SEARCH is not available on Spark 3.4 is slightly off: the 3.4 module registers SEARCH identically to 3.5/4.0 (no version gating), so the function is visible. What actually blocks it is that search_columns is required and cannot be passed positionally, only as a named argument, and named arguments need Spark 3.5+.

The conclusion (unusable on 3.4) is right, but "not available" reads as "not registered". Consider "cannot be used on Spark 3.4", or spell out that it is registered but uncallable because 3.4 lacks named-argument support.

@disabled

The Validation section leaned on xfail/XPASS/@disabled and read as "tested and passing", while the Basic Usage example selects _score and therefore fails on a queryTable-capable namespace. Lead with that instead, in the words the repo's own xfail reason uses, and collapse the CI mechanics into one coverage line. Also correct "not available on Spark 3.4": the 3.4 module injects the search table function identically to 3.5 and 4.x, so it is registered but uncallable there.

I added a warning saying the server-side route cannot produce _score, taking the xfail reason and the review note as fact without reproducing it. Running the disabled JVM case on this head shows a different failure: it throws "SEARCH requires search_columns for full-text search" at analysis time, because the case uses the positional form. It never reaches execution, so nothing here demonstrates the _score claim. Remove the warning and the matching Validation claim, and state only the coverage the workflow provides. The Spark 3.4 rewording stays.

lance-gatekeeper

✅ Gate recommendation: approve.

The latest revision removes the unproven server-side failure warning and matching validation claim. The SEARCH argument, Spark 3.4 availability, execution-route, and validation-coverage corrections now match the implementation and verified behavior.

github-actions Bot added the documentation Improvements or additions to documentation label Aug 26, 2026