Repository navigation
feat(python-sdk): add managed file queries and Arrow analytics - #30054
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — design-phase gate failed on an unclosed external-table ownership transition. Reviewed all four versioned designs, their acceptance matrices, the change map and causal construction/cleanup paths. This is a new cross-owner persistent/streaming feature; design review is mandatory, and all four documents currently mark acceptance pending. The inline counterexample must be closed in the proposal before implementation approval. A narrow fault-injection probe through the actual manager/cleanup code confirms that a modeled committed CREATE with a lost acknowledgement leaves the generated table unregistered, whereas the normal control issues CREATE and DROP. This is not a new native lost-ack run: schema and commit-boundary dependencies are injected. Existing native upload COMMIT faults and the pre-second-CREATE rejection test do not cover this construction boundary. Preserve owned names before ambiguous side effects and retain scoped recovery/cleanup evidence without blind retry or a new reaper. Full downstream implementation/test review is gated by this design failure; no claim that all 21 files have passed implementation review. No source edits, subagents or merge.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: c7159569c1607ad32b1810d8e79ff9debf5e344a
Assessment: request_changes
Inspected the complete 22-file delta and four designs. The construction-ownership fix passes focused models, but TIME conversion corrupts supported values and the runnable example can delete a pre-existing table.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
-
P2
clients/python/matrixone/analytics.py:374: Read a Parquet time32('ms') column through files.query(), or request time32('s'/'ms') or time64('ns') in analytics.query's result schema.
Impact: _time_microseconds produces microseconds, but pa.array interprets those integers in the requested unit. Thus 00:00:02 becomes 2,000,000 milliseconds instead of 2,000. Fractional values also bypass checked narrowing. Construct a microsecond-typed source array before safely casting to the requested type.
Evidence: Precision probes reproduce three unit mismatches and accepted lossy narrowing; the microsecond control passes. test_review_default_time.py reproduces the default Parquet-query failure through real SDK/Arrow code with an injected TIME transport. Evidence: default-time.log and checks/1792312493030524337-3.json. -
P2
clients/python/examples/example_35_arrow_analytics.py:46: Run the example against a database containing analytics_example. Its default create=True upload rejects the existing table.
Impact: The finally block nevertheless drops analytics_example, deleting an object the example never created. Use a unique owned name and distinguish collision rejection from owned creation before cleanup. benchmark_analytics.py:128 repeats this fixed-name cleanup pattern.
Evidence: The actual example.main, exercised with a SQLite-backed CREATE/DROP boundary, deletes a pre-existing table containing row 42 after CREATE fails. The owned-table cleanup control passes. Evidence: example-final.log and checks/17923124224484690-5.json; the authoritative receipt mapping is retained in review-evidence.json.
Validation:
-
Preflight passed. Python 3.13.5, PyArrow 21.0.0 and SQLAlchemy 2.0.55: 22 focused ownership/inference/type tests passed without skips, including eight construction/collision cases. Evidence: ownership.log and checks/1792312354401521984-3.json.
-
The prior lost-CREATE-ack concern is addressed by reservation-before-CREATE, collision exclusion and retryable scoped cleanup in source and focused models.
-
Retained regression probes include neighboring controls. Initial example harness failures are distinguished from product failures. Additional upload-cast observations were not promoted to findings because their contract is less explicit.
-
Commands, source fingerprints, terminal receipts, JUnit outcomes and logs were reconciled in review-evidence.json under /home/xupeng/.local/state/mo-bot/jobs/fe876c8964b0f3af2635f952/attempt-1. Source remains unchanged.
-
Frozen evidence records six successful exact-head SDK offline CI cells. Online/kernel checks were skipped; local_validation contains no reusable worker gates.
Limitations:
-
Local probes use injected database boundaries with real SDK/Arrow execution, or SQLite for example ownership. Native MatrixOne, S3, multi-CN, soak and crash campaigns were not rerun.
-
Historical author reports reference unavailable raw XML/native artifacts; their native and performance outcomes were not independently reconciled to this candidate.
-
The four design documents still mark acceptance pending. This assessment does not grant design acceptance or full native integration approval.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: 84792be374c46591eb249986ac33c05ebb6f13b7
Assessment: request_changes
The prior TIME-conversion and example-cleanup counterexamples now pass focused checks. However, mandatory design acceptance remains open; full implementation approval is gated.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
- P2
clients/python/docs/design/analytics-file-lifecycle.md:71: This new analytics feature adds an 887-line production module and crosses persistent catalog, shared staging, transaction and streaming ownership boundaries.
Impact: The repository requires a distinct design-first approval before implementation acceptance. Record a traceable approval decision for the exact four design revisions, then complete downstream implementation review; passing tests cannot substitute for this gate.
Evidence: feature-design-review.md sections 1–2 and 4 explicitly require this gate. All four designs state that design review and merge acceptance are pending: lifecycle:71, inference:68, upload:63 and stream:57. The current PR description also states that parent design acceptance remains open; supplied reviews contain no subsequent approval.
Validation:
-
15 focused tests passed, with seven native cases deliberately deselected: six TIME conversion cases, eight CLI ownership cases and the retained default-Parquet TIME counterexample. fixes.log/fixes.xml match checks/1792326171177093328-3.json.
-
Reconciled retained receipts, logs and JUnit results for 22 ownership/inference/type models, verifying relevant source and test inputs remain unchanged. Audit receipt: checks/1792326176819707215-4.json.
-
Preflight and final evidence audit passed; source remained unchanged. Four designs and the complete follow-up delta were reviewed. Frozen evidence records six successful offline SDK CI cells.
-
Scope and evidence mapping: /home/xupeng/.local/state/mo-bot/jobs/9ba21d667bd0ade7f7afc99f/attempt-1/review-record.json.
Limitations:
-
Full downstream implementation, broader test architecture, performance and security review remain pending under the mandatory design gate.
-
Focused checks use real Arrow with injected database boundaries or SQLite ownership operations; they do not establish native MatrixOne acceptance.
-
Current native, S3, multi-CN, soak and crash claims were not independently reconciled. local_validation is empty, historical raw XML artifacts were unavailable, and online CI was skipped.
aptend
left a comment
There was a problem hiding this comment.
Reviewed exact head 84792be374c46591eb249986ac33c05ebb6f13b7 against base 8ed73ff81531db6ec72cdb4eb791b7889d3b44b6. REQUEST_CHANGES for one independently reproduced persistent-ownership defect.
[P2] Freeze the schema as part of external-table cleanup ownership — clients/python/matrixone/analytics.py:516.
FileRelation retains the engine and UUID table name, but its DROP is unqualified. A pooled connection's current database can change independently of its engine URL. With a single-connection pool:
c = Client(..., database="a", pool_size=1, max_overflow=0)
r = c.analytics.read_parquet(path)
# n is the generated mo_py_files_<uuid> table in a.
# An independent connection may create b.n containing sentinel row 42.
c.execute("USE b")
r.close()The close returns successfully and clears _tables / sets _closed=True, yet independent catalog checks show a.n still exists. If b.n exists, that unrelated table is deleted. Without the b.n control, DROP IF EXISTS still falsely retires ownership while leaking a.n. No network fault, crash, UUID collision during CREATE, or client reconnect is required.
This reproduced against a freshly built native CN from this exact head. A second public-API run repeated three pairs: all three no-USE controls dropped only a.n and preserved b.n; all three USE cases leaked a.n, deleted b.n and reported closed/no pending ownership.
Capture the actual schema on the creating connection before ambiguous CREATE/COMMIT, retain schema+table as the object identity, and qualify owned relation references/cleanup with it. Retaining the original engine alone is insufficient; reading only its configured URL is also insufficient if that session was already switched. Add USE/default-schema reuse and same-name foreign-table controls alongside existing lost-ack/reconnect cases.
Review scope and evidence:
- Complete 24-file implementation/test/doc review, all 3 reviews, 3 discussion comments, inline comment and unresolved/outdated thread; also the latest 7-file correction and both parent contracts.
- Independently reviewed all four exact-head designs before implementation validation: inference
fa33bb723e8ba3ad5fd1478f906aca0d0d95b77b, lifecyclec1e14cb0f045c7ac301491cafe0240f56a5041cb, uploadd09b12635bc608aeaf36e203996f896eb80221a1, stream6bf0728c639db06bee783dde1b572bb383786d91. The ownership identity needs the concrete correction above. This verdict is not based on pending approval labels/text. - Prior construction reservation, TIME-rescaling/precision and CLI collision findings pass fresh applicable tests.
- Python 3.13.5 / Arrow 26.0.0 / SQLAlchemy 2.0.55 / PyMySQL 1.2.3: full SDK offline + analytics collection 1071 passed + 86 subtests, 187 explicit environment skips.
- Fresh native single-CN analytics run: 204 passed, 3 failed, 16 skipped. Lost CREATE/COMMIT acknowledgements, collision preservation, strict import/upload rollback, stream timeout/pool recovery and TIME cases ran. The three permission-suite failures are server rejection of tenant SQL-file paths (only sys moadmin allowed); no permission bypass attempted, and these are not counted as passing. Multi-CN/S3 were skipped. Test endpoint literals were remapped only to this harness-owned CN; no existing local service was used.
- Exact-head CI has six successful offline cells; online/kernel jobs were skipped. Historical S3/dual-CN/soak/SIGKILL evidence was inspected but not relabeled fresh. No full native-acceptance, performance or cloud certification.
| Unhappy-path audit | Closure |
|---|---|
| Q1 ownership | Staging and stream owners are explicit; persistent relation owner is missing its schema component. |
| Q2 cleanup/failure | Lost-ack/rollback/cleanup-state controls checked; normal close can delete the wrong object and discard recovery evidence. |
| Q3 reuse/restart/growth | Real pooled-session database reuse reproduces this finding; early-close/FD/concurrency controls exercised; crash/soak campaigns not rerun. |
No production source changes or pushes. All counterexample databases were removed in finally and the isolated native fixtures closed.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 9907136d5063cc152f26a263e13474078d8de945
Assessment: request_changes
Reviewed the complete 24-file delta, four designs and prior concerns. Reusing native scans/LOAD and shared lifecycle helpers is reasonable, but database identity is missing from persistent-table ownership. This defect blocks design acceptance.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
- P2
clients/python/matrixone/analytics.py:516: Create a file relation in database a using a single-connection pool, call client.execute("USE b"), then relation.close().
Impact: The unqualified DROP targets b, leaves the owned table in a, and clears its recovery reservation. If b contains a same-named table, cleanup deletes that unrelated table.
Evidence: Independently reproduced through the current SDK with real SQLAlchemy pooling and SQLite-backed catalogs modeling USE/CREATE EXTERNAL semantics. The no-USE control preserves foreign data; both USE cases leak the owned table, and the same-name case deletes foreign data. MatrixOne's buildDropTableSingle also resolves unqualified names using the current database. Capture the actual creating connection's schema before CREATE/COMMIT uncertainty and retain qualified identities for relation SQL and cleanup; add database-switch and foreign-object controls.
Validation:
-
22 focused tests passed; seven native tests were deselected. These cover prior construction-ownership, TIME-unit and example/benchmark cleanup fixes.
-
Ownership probe confirmed three control/counterexample outcomes; it was a boundary model, not native MatrixOne execution.
-
Reconciled commands, pinned HEAD, clean-source fingerprints and exit-zero receipts: checks/1792669689366159112-4.json (probe), checks/1792669713978273799-3.json (focused tests), checks/1792669919208709292-4.json (evidence audit).
-
Logs, XML, probe and hashed review record are retained under /home/xupeng/.local/state/mo-bot/jobs/22e12de4360cdd7beae11bc6/attempt-1/.
-
Frozen CI evidence reports six successful SDK offline cells; online CI was skipped.
Limitations:
-
Full implementation acceptance remains gated on ownership/design closure. No fresh native-server, permissions, multi-CN, S3 or chaos campaign was run.
-
local_validation is empty and referenced earlier artifact directories are unavailable; historical native and performance reports were not independently certified.
-
Streaming should reduce retained result rows, while file relations add per-file catalog work and repeated CSV validation I/O. Ordinary SQL paths remain unchanged; no fresh performance measurements were taken.
-
Linked issue bodies were absent; behavioral contracts were assessed through the frozen PR and versioned designs.
9907136 to
a8d6e0b
Compare
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: a8d6e0b54b000ec770e0ac57878ff78797945308
Assessment: approve
Reviewed the complete 24-file delta. The design coherently reuses native external tables, Arrow LOAD and existing connection ownership. Qualified catalog identities, transaction outcomes and stream cleanup address the prior concerns. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
Validation:
-
Fresh frozen-head native build: all 247 analytics tests passed without skips on both Arrow 21/SQLAlchemy 2.0.55 and Arrow 18/SQLAlchemy 1.4.54, including two CNs, S3, tenant permissions, packet faults and ownership regressions.
-
Offline validation: 1,572 initial passes and 95 passing subtests. Five missing-package-entry-point failures were resolved; the affected 83-test rerun passed.
-
Black, isort, configured fatal Flake8, whitespace checks, wheel/sdist builds, Twine and optional-dependency/package-content checks passed.
-
250,000-row comparison preserved counts/checksums: streamed additional peak RSS was 2.9–3.2 MiB versus approximately 80 MiB buffered. Timing varied; no throughput improvement is claimed.
-
Reconciled commands, frozen-source fingerprints, exit receipts and retained logs in /home/xupeng/.local/state/mo-bot/jobs/265886197afb95350b6939ea/attempt-1/validation-manifest.json. Detailed assessment: review-report.txt. Source remained unchanged.
Limitations:
-
No endurance soak, SIGKILL/restart campaign, million-row constrained-memory run, cloud certification or cross-host failover validation was performed; these remain explicit qualification limits.
-
Linked-issue bodies were absent from the supplied evidence; requirements were assessed against the PR description and four versioned designs. Unavailable historical author results were not counted as verified.
Merge Queue Status
This pull request spent 13 minutes 49 seconds in the queue, including 1 minute 41 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
Closes #30050
Closes #30051
Closes #30052
Closes #30053
Tracks parent features #30048 and #30049; broader parent acceptance remains open.
What this PR does / why we need it:
Add optional synchronous file exploration through managed ordinary external tables, DataFrame/Arrow uploads through native Arrow IPC LOAD, and bounded Arrow result batches through an unbuffered PyMySQL cursor. PyArrow remains an optional dependency.
A relation previously kept bare table names. After
Client.execute("USE other_db"), querying or closing it could access another database's same-named table, delete that foreign object, and leave the owned external table behind. CaptureSELECT DATABASE()on each actual CREATE connection and reserve the exact(database, table)before CREATE/COMMIT. Quote both identifier components for CREATE, SELECT and DROP, retaining the original engine for query, import and cleanup. Definite name collisions preserve the foreign object; uncertain outcomes retain a scoped recovery owner without retrying CREATE. Failed cleanup attaches its error while preserving an existing primary exception, including when logging also fails.Keep checked scalar mapping, strict transaction/session restoration, unknown-commit diagnostics and checked TIME unit conversion. Each source adds one database-identity query; no throughput improvement is claimed.
Clean up the delivery: remove unused mapping flags, unrelated Client formatting changes, stale generated validation JSON and historical measurement claims. Preserve useful benchmark, soak and crash oracles. Adapt crash recovery to qualified identities, authorize harness DROP only after acknowledged creation, and share the duplicated memory-pressure stage. Regression oracles independently inspect real SQLite/native catalogs and preserve foreign sentinel rows.
Rebase the final feature delta onto main
9dcd26be433018931842f406e915207bc6aa175f, preserving the original feature author and excluding unrelated historical Go changes. The design-only commit precedes the repair implementation. The issue-to-pr design and complete final review usegpt-6.1-sol/xhigh; this is separate from GitHub approval. Design approvalPR30054-DESIGN-2026-10-25is committed at d25811b; final overall reviewPR30054-OVERALL-2026-10-25-FINALpasses revisiona8d6e0b54b000ec770e0ac57878ff78797945308with no unresolved material blocker.Design and acceptance contracts:
Validation
Python 3.13.5, PyArrow 21.0.0, SQLAlchemy 2.1.6, PyMySQL 1.2.3; DuckDB 2.0.0, pandas 3.1.0 and Polars 2.0.0. Actual dependency identities, commands, logs and JUnit results are retained outside the source tree.
fetchone; the scalar oracle now usesfetchall()[0]. A BOOL CAST is advertised as TINYINT by this server, so tests verify that protocol type and an explicit Arrow bool schema. Tenant external metadata creation succeeds but local-path scanning requires sys/moadmin, which the permission tests now assert with a positive sys control. Local S3 tests require loopback proxy bypass. All affected cases pass after these corrections; initial failures are not counted as passes.Native acceptance used an isolated, owned, same-host dual-CN deployment of the identified prebuilt main
12eeeb0205b5e87faef48660ddbcd8cb5e18baeaproduction source. This verifies the current SDK against that compatible server; it is not a fresh build of the rebased kernel. Minimum-version dependency matrices, million-row pressure, soak and SIGKILL campaigns were not rerun, and unavailable historical reports are not presented as current evidence. Runnable commands and prerequisites.CI for the updated head is pending; no merge or GitHub approval is claimed.
Limits
External tables require CREATE/DROP privileges and explicit cleanup; crashes can leave catalog objects. Native Arrow LOAD requires client/CN-visible staging. Output uses MySQL rows converted to Arrow, with row-bounded batches rather than a universal byte bound. Network timeouts bound inactivity rather than total query duration. Source preflight is not an atomic snapshot. Unknown commits and SIGKILL artifacts require scoped reconciliation. Local S3 and same-host CN checks do not certify cloud providers, cross-host failover or endurance.