Skip to content

feat: Add async REST scan planning poll and plan storage credentials - #3724

Open
lukeFalsina wants to merge 4 commits into
apache:mainfrom
lukeFalsina:feat/async-rest-scan-planning
Open

feat: Add async REST scan planning poll and plan storage credentials#3724
lukeFalsina wants to merge 4 commits into
apache:mainfrom
lukeFalsina:feat/async-rest-scan-planning

Conversation

@lukeFalsina

@lukeFalsina lukeFalsina commented Jul 31, 2026

Copy link
Copy Markdown

Summary

  • Implement async REST fetchPlanningResult / cancelPlanning polling when planTableScan returns status=submitted
  • Apply completed-plan storage-credentials to the scan-scoped FileIO (layered on existing IO properties)
  • Keep public RestCatalog.plan_scan(...) -> list[FileScanTask] unchanged; credentials flow through internal _plan_scan_result / _file_io_from_plan
  • Honor per-table scan-planning-mode from LoadTableResponse.config, which takes precedence over the catalog-level / GET /v1/config setting (same precedence as Java)

Related: #2775, #3495
Java reference: apache/iceberg#13400 (async planning), apache/iceberg#15572 (table-level scan planning override)

Rationale

Unblocks REST catalogs that return async plans (for example policy-protected tables). Finishes the unchecked async items from #2775 and the plan-credential gap from #3495.

Per-table loadTable overrides let a server request Scan Plan API only where needed (e.g. policy-protected tables) while other tables keep client-side planning, without forcing a catalog-wide scan-planning-mode=server.

User-facing

  • table.scan() with scan-planning-mode=server now handles async plans automatically
  • New optional rest-scan-planning.poll-timeout-ms (default 300000)
  • No breaking change to RestCatalog.plan_scan return type
  • Effective scan planning mode resolution:
    1. scan-planning-mode in the table's loadTable response config (wins when present)
    2. Catalog-level client / GET /v1/config property
    3. Default client
  • Plan endpoint must still be advertised for server-side planning
  • Invalid catalog-level scan-planning-mode values are ignored with a warning (they cannot block a valid loadTable override or the default); invalid loadTable values still raise

Test plan

  • make lint
  • make test (3812+ passed)
  • Unit tests in tests/catalog/test_scan_planning_models.py for poll success / timeout / failed / cancelled and IO property retention
  • Unit tests for loadTable override precedence, client default without override, and invalid catalog mode surviving a valid table override
  • Existing REST scan integration suite (CI)
  • Manual validation against an async-capable catalog (Snowflake Horizon Iceberg REST Catalog Scan Plan API), including mixed protected / unprotected tables in one session without a catalog-wide scan-planning-mode

Made with Cursor

@singhpk234 singhpk234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lukeFalsina this is really promising, have some suggestions inline

Comment thread mkdocs/docs/api.md Outdated
Comment thread mkdocs/docs/configuration.md Outdated
Comment thread pyiceberg/catalog/rest/__init__.py
Comment thread pyiceberg/catalog/rest/__init__.py
@lukeFalsina
lukeFalsina requested a review from singhpk234 August 3, 2026 13:20
@singhpk234

singhpk234 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Thanks @lukeFalsina i think we are getting pretty close ! i think we should also add support for this scenario for this pr :
#3724 (comment), let wdyt ?

iiuc for your testing you added a client side config to always do remote scan planning right ?

@singhpk234
singhpk234 requested a review from HonahX August 14, 2026 18:43
@lukeFalsina

Copy link
Copy Markdown
Author

@singhpk234 Yes — initially in my testing I was setting the catalog-wide scan-planning-mode=server from the PyIceberg client.

What you proposed makes more sense, so I've updated the PR to support it: the loadTable config override takes precedence over the catalog-level setting.

I also re-tested with the catalog-wide property unset and only the per-table loadTable configs. That is much simpler from a user perspective, and it worked well in a single script that read both a policy-protected and an unprotected table from the same catalog — I verified the client used the plan choice returned by the server correctly for both table types.

Details are in #3724 (comment) and the latest commit on this PR.

@lukeFalsina

This comment was marked as outdated.

@singhpk234 singhpk234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @lukeFalsina !

added some minor suggestions

Comment thread mkdocs/docs/configuration.md Outdated
Comment thread mkdocs/docs/configuration.md Outdated
lukeFalsina and others added 4 commits August 19, 2026 09:41
Catalogs that return status=submitted from planTableScan can now be polled via
GET .../plan/{plan-id}, with best-effort cancel and scan-scoped FileIO rebuilt
from plan storage-credentials. Public RestCatalog.plan_scan still returns
list[FileScanTask].

Co-authored-by: Cursor <cursoragent@cursor.com>
Address review feedback: note scan-planning-mode can come from catalog
config, document async poll until terminal state, and restore the
expand-plan-tasks section comments.

Co-authored-by: Cursor <cursoragent@cursor.com>
Prefer scan-planning-mode from LoadTableResponse.config over the
catalog-level property (matching Java), so REST catalogs can enable
server-side planning only for selected tables. Invalid catalog values
are ignored with a warning and no longer block a valid table override.

Co-authored-by: Cursor <cursoragent@cursor.com>
Clarify async plans poll until a terminal state, and note that plan
storage-credentials are the creds vended by the server.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lukeFalsina
lukeFalsina force-pushed the feat/async-rest-scan-planning branch from ac5caab to 6ead3e9 Compare August 19, 2026 07:43
@singhpk234 singhpk234 closed this Aug 19, 2026
@singhpk234 singhpk234 reopened this Aug 19, 2026
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.

2 participants