Skip to content

[refactor](lance) improve lance catalog implementation in FE - #66581

Merged
yiguolei merged 2 commits into
apache:branch-4.1from
zhangstar333:lance-refactor-code
Aug 7, 2026
Merged

[refactor](lance) improve lance catalog implementation in FE#66581
yiguolei merged 2 commits into
apache:branch-4.1from
zhangstar333:lance-refactor-code

Conversation

@zhangstar333

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary:
refactor some code including:

  • Adds dedicated properties for filesystem and REST catalogs.
  • Uses the Lance tableExists API for table existence checks.
  • Refactors metadata loading and snapshot version resolution.
  • Improves method naming and documentation for readability.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@zhangstar333
zhangstar333 requested a review from yiguolei as a code owner August 7, 2026 09:19
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@zhangstar333

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor

I found one blocking compatibility issue with the tableExist change.

LanceExternalCatalog.tableExist() now calls namespace.tableExists(), which uses POST /v1/table/:id/exists for a REST namespace. However, the existing Doris Lance REST fixture (docker/thirdparties/docker-compose/iceberg/scripts/lance_rest_server.py) only implements ListNamespaces, ListTables, and DescribeTable; every other POST request returns 404. The unit-test mock in LanceRestCatalogTest has the same limitation.

Consequently, a table that can be listed and described successfully by the currently supported REST fixture can be reported as non-existent (or fail with an exception, depending on the SDK error mapping) whenever a caller uses tableExist().

Please update the REST fixture and unit-test mock to implement the table-exists endpoint, and add coverage for:

  • an existing table;
  • a missing table/namespace;
  • authentication or service errors, which must not be converted to false.

If Doris intends to remain compatible with namespace implementations that support List/Describe but not TableExists, please also fall back to describeTable() when TableExists is unsupported.

Also, the PR checklist currently says that this is a pure refactor with no behavior change and no tests needed. That is inaccurate: the previous implementation converted every RuntimeException to false, while the new implementation only converts explicit not-found errors and propagates other failures. This is a desirable behavior change, but it should be documented in the PR description and covered by tests.

@zhangstar333

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 66.67% (160/240) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 59.17% (142/240) 🎉
Increment coverage report
Complete coverage report

@yiguolei
yiguolei merged commit 8375559 into apache:branch-4.1 Aug 7, 2026
29 of 32 checks passed
yiguolei pushed a commit that referenced this pull request Aug 14, 2026
### What problem does this PR solve?

Issue Number: N/A

Related PR: #65730, #66581

Problem Summary:

Ordinary Lance scans currently read **every row of a fragment** even
when the
query only needs the first N rows (e.g. `SELECT ... LIMIT 10`). Lance
applies
its own LIMIT *after* the scanner's filter, so the query LIMIT can be
forwarded
to each fragment scanner and let it stop early, cutting IO and decode
cost.

**How it is fixed**

- `thrift`: add an optional `TLanceFileDesc.limit`.
- `FE` (`LanceScanNode`): push the query limit into each fragment split
via
  `canPushDownLimit()`, and surface `lanceLimit` in the explain output.
- `BE` (`lance_reader`): forward it to the scanner through
`lance_scanner_set_limit` for ordinary scans; vector search keeps its
own
  `top_k` limit.

**Correctness**

The limit is pushed **only when all predicates are already pushed into
Lance**
(no residual Doris conjunct). Otherwise Doris still re-filters the
returned rows,
and truncating a fragment early could drop valid results.

`OFFSET` needs no special handling: Nereids' `SplitLimit` rewrites
`Limit(limit, offset)` into a global `Limit(limit, offset)` over a local
`Limit(limit + offset, 0)`, and that local bound is what reaches the
scan node.
So `getLimit()` already includes the offset; each fragment fetches up to
`limit + offset` rows and the upper global LIMIT still applies the
offset and
the final bound. Per-fragment truncation is therefore always safe.

**Behavior change**

Query results are unchanged. Only the number of rows scanned per
fragment is
reduced for LIMIT queries; the explain output shows an extra
`lanceLimit=N`
line when the limit is pushed.

### Release note

Push down LIMIT into Lance fragment scanners to reduce the rows scanned
for
`LIMIT` / `LIMIT ... OFFSET` queries over Lance tables.

### Check List (For Author)

- Test
- [x] Unit Test (`LanceThriftContractTest` covers the limit round-trip
and the no-limit case)
- [ ] Manual test — `SELECT * FROM <lance_tbl> LIMIT 10` returns 10 rows
and `EXPLAIN` shows `lanceLimit=10`; a query with a non-pushable
predicate keeps the limit out of the scan
- Behavior changed:
  - [x] No.
- Does this need documentation?
  - [x] No.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants