Repository navigation
Conversation
…30075) ## What type of PR is this? - [x] Improvement - [x] Test and CI ## Which issue(s) this PR fixes: Closes #30064 Fixes #30078 ## What this PR does / why we need it: Move SDK validation into the repository-root Actions directory. Replace undiscovered nested workflows and missing/mismatched requirement paths with installed-package offline and distribution checks for Python 3.10/3.12 with SQLAlchemy 1.4/2.0, and Python 3.11/3.12 with SQLAlchemy 2.1. Package installation registers the dialect entry point; SQLAlchemy's asyncio extra supplies the required runtime. Include offline analytics cases when that directory is present. Add an explicitly invoked online job for a configured isolated self-hosted runner/environment; failures are not suppressed and JUnit summaries expose skipped cases. No runner, environment or credential is provisioned here. SDK_CI.md documents prerequisites, and this selected matrix does not certify advertised Python 3.8/3.9 support. Adapt the existing reserved-metadata test to SQLAlchemy 2.1's warning contract while retaining the exception assertion for 1.4/2.0; no blanket xfail/skip is added. ## Validation - Full installed-package offline suite: 977 passed plus 86 unittest subtests independently on SQLAlchemy 1.4.54, 2.0.54 and 2.1.4. - Parsed workflow YAML; verified PR/push/dispatch triggers and all matrix entries; bash -n passed for each embedded run script. - No continue-on-error remains in the new workflow. Online execution is only an explicit dispatch against a configured test deployment. - Local validation used Python 3.12; GitHub CI now independently passed all six supported Python 3.10/3.11/3.12 matrix cells; the local run alone was not used to claim those runtimes. - Wheel/sdist checks are part of the workflow; local wheel building is separately verified. All reported integration runs use the isolated local MatrixOne source build `cd8d5abf9665a97d13e50b04840fb74b877fd339`, synthetic data and (for TLS) local test certificates. They do not certify a production deployment or a released server binary. These are draft PRs for review; no merge is performed. The four fixes are tested together as well as independently, and integration with #30054 is being verified. GitHub CI status is separate from the recorded local results. ### Combined integration validation All four changes plus the analytics implementation from #30054 were tested together: **1258 passed, 86 unittest subtests passed, zero failures/errors/skips** in 38.38 seconds (JUnit counts 1344 including the subtests). This includes the installed-package offline suite, all 192 analytics cases with S3/two-CN prerequisites, and 30 new native regression cases. Bulk tests use two local plaintext CNs; strict certificate/no-downgrade cases use a separate dedicated TLS CN. The earlier TLS stress failure remains recorded and linked to the existing server issue. This is a local integration result, not GitHub CI completion. ### GitHub matrix correction The first run passed five matrix cells; Python 3.10 / SQLAlchemy 2.1 failed during dependency installation because SQLAlchemy 2.1 declares Requires-Python >=3.11. Commit da01818 replaces that unsupported cell with Python 3.11 / SQLAlchemy 2.1, preserving six valid cells. The failed first run is retained: https://github.com/matrixorigin/matrixone/actions/runs/38336849870 . All six supported SDK matrix jobs passed in the corrected run: https://github.com/matrixorigin/matrixone/actions/runs/38338130924 . The manually invoked online job was not run; local online evidence is reported separately. ### SDK-only CI routing Extend the existing trusted planner with SDK-only routing instead of a second classifier. `clients/python/**` plus ordinary docs runs the SDK checks and omits kernel build, UT, coverage, BVT, upgrade and utils. Mixed SDK/kernel changes retain both suites. SDK CI control changes, force-full and untrustworthy file lists require full kernel plus SDK checks; rename impact follows both paths. The root workflow is the sole reusable automatic SDK PR owner, called with contents-read permissions and no inherited secrets. Offline jobs check out immutable fork-head code on hosted runners without persisted credentials. Manual credentialed online execution stays separate. Distinct concurrency groups, bounded offline execution and per-run/attempt/cell artifacts avoid duplicate ownership and identity collisions. CI Required rejects missing or unsuccessful required SDK results. Routing evidence: 31 Node contract tests, incremental workflow/static checks and fatal Python lint passed. The six-cell SDK run linked above remains evidence for its exact parent revision; it is not a live qualification of the modified routing. ### Documentation-only CI routing Recognize ordinary Markdown (`.md`) and reStructuredText (`.rst`) documentation throughout the repository, with case-insensitive extensions, instead of requiring one of four root names or a path under `docs/`. Build guides, package READMEs, skill documentation and RST pages now use the existing documentation whitespace check and metadata/size-label job, without kernel build, UT, coverage, BVT or upgrade execution. Keep the existing classifier as the sole owner. SDK documentation retains SDK checks because its README is consumed by packaging. Exact `testdata/` and `fixtures/` directory segments retain their input owner (SDK within `clients/python/`, full kernel otherwise); lookalike names remain ordinary docs. Executable MDX, text inputs, scripts/configuration and images outside `docs/` are not exempted. Mixed changes and both rename paths retain their required suites, and untrustworthy file lists or forced-full retain conservative full+SDK behavior. Validation: all 31 owning Node contract tests passed with zero failures/skips/cancellations; 251 composed checks execute the actual preflight script, classifier, workflow admission conditions and required-result verifier. Required unsuccessful/missing results are rejected. A real committed Git diff with a sparse `docs` checkout accepts a clean package README absent from the worktree and rejects both trailing whitespace and blank-at-EOF damage. Node syntax and incremental whitespace checks passed. The exact-head hosted [CI scope contract](https://github.com/matrixorigin/matrixone/actions/runs/38358226404) also passed. Existing fixtures/helpers are reused; no new workflow, API scan, stored state or server/SDK runtime work is introduced. The required design and final overall review gates completed before publication. No full kernel CI was run for this classifier follow-up; its automatically triggered full run is cancelled under the requested scope, while the lightweight contract workflow is retained. Live routing becomes effective only after deployment to the trusted target branch; this PR is not merged here. ### Online review fixes and current validation All actual default/hardcoded online connection entry points now consume the existing OnlineTestConfig snapshot, including partial-default JSON setup and duplicated Client constructor mappings. Fixture-created Pinecone databases preserve their explicit database selection while using configured host/auth; ORM bootstrap and cleanup use the configured base database, with its worker using the fixture database. No SDK production code or TP execution path is changed. The small kwargs adapter creates a fresh mapping only during test connection setup, with no new configuration store, cache or retries. Move four pure configuration oracles to the offline layer. Scoped pytest monkeypatch contexts restore both environment values and key absence after success, assertion failure and constructor failure; unused saved fields and manual cleanup are removed. - **Current-code full offline suite:** 1035 tests plus 86 unittest subtests passed separately with SQLAlchemy 1.4.54 and 2.0.44 on Python 3.10.18. - **Focused strict-mode regressions:** 58 passed, covering every migrated real constructor/method/unwrapped fixture under remote/default configuration, fixture database ownership through ORM cleanup, connection failure without fallback, exact environment restoration and subsequent consumer/configuration generations. These probes stop at the connection boundary and make no database connection. - **Incremental SCA:** syntax, configured Black/isort with exact pre-fix baseline comparison, fatal flake8 and whitespace checks passed. New tests are formatted; no new format violations are introduced into existing files. - Required gpt-6.1-sol/xhigh design and final overall review gates completed before publishing the reviewed content. The historical six-cell offline result does not validate these new/moved tests; the fresh local results above do. Matrix/dependency definitions and packaged SDK production contents are unchanged. Credentialed online SQL execution and live SDK-only routing after trusted-main deployment remain pending rollout qualification. Go race/kernel/CGo/BVT checks are inapplicable; deterministic scoped restoration/failure/sequence tests cover the affected Python state, for which race instrumentation is unsupported. Full kernel CI automatically triggered by this update is cancelled per the requested scope and is not reported as passing. The logservice/ISCP records below are retained from parent `dcf542fe5d41985edfdceaad592acbf096800f3e`. Their kernel CI status is separate from this Python-only follow-up; they are not replaced by the Python evidence above. ### Shared logservice CI compilation repair The original SCA and UT coverage failures shared the official-main defect #30047: the gossip fixture called an obsolete allocator signature. This branch includes the small, tested repair from #30083 so its head can compile without waiting for upstream rollout. The original SDK implementation is unchanged. Validation: clean-main compiler reproduction, fixed Go vet, existing port/gossip collision/cancellation/rollback tests passed (95.107s), and the new three-node range/uniqueness regression passed 20 times (0.082s). All tests retain their assertions. The separate ISCP failure from an earlier #30075 coverage run is being investigated; its isolated native reproduction passed on latest main (34.828s). New full CI results remain pending. This follow-up retains the SDK routing commit 93b33e8. The new head also contains the logservice test repair, so the latest kernel CI triggered by this follow-up validates that Go change; earlier cancelled runs are not counted as passes. ### ISCP failure reproduction follow-up The retained earlier coverage artifact shows `TestIssue29400DropDatabaseRCReclaimFailureRollsBackWholeDatabase` timing out at the initial ISCP-consumer readiness assertion (line 1091), with preceding lease-reader errors `unexpected rows count: 8192`. The isolated scenario passes on clean latest-main source (34.828s), and three consecutive runs also pass using `-covermode=set` with the exact cross-package instrumentation scope from that CI artifact (76.768s). This exercises instrumented production code; it is distinct from an earlier test-package-only coverage attempt whose output explicitly had no statements. These passes do not reproduce the full-suite concurrency/order conditions, and do not establish that the earlier failure is fixed. No ISCP assertion, timeout, lease uniqueness check or production code has been changed. The new full CI run remains authoritative. --------- Co-authored-by: XuPeng-SH <xupeng3112@163.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
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.
| params.extend([delimiter, '"']) | ||
| with self.engine.begin() as conn: | ||
| conn.exec_driver_sql(ddl, tuple(params)) | ||
| tables.append(table) |
There was a problem hiding this comment.
[P2] Register table ownership before CREATE can have an uncertain outcome
The table is added only after engine.begin().exit returns. If CREATE EXTERNAL TABLE commits but its COMMIT acknowledgement is lost, this append is never reached; construction enters relation.close() with no record of this UUID. The caller receives only the failure and never obtains the relation, so a normal network error can leave an ordinary catalog table with no cleanup/recovery owner. The SIGKILL limitation does not cover this ordinary exception path.
A fault-injection probe through the real read_files/FileRelation path produces CREATE+DROP normally and CREATE with no DROP after the modeled post-commit acknowledgement failure. Existing packet fault tests cover upload_arrow, while partial_relation_creation_cleanup fails before the second CREATE. Reserve the uniquely owned name before ambiguous side effects, attempt cleanup on a fresh usable connection, and preserve the scoped name/cleanup evidence if outcome cannot be confirmed. Amend the lifecycle design and extend the native commit-ack control to this entry point.
|
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.
| for table in list(reversed(self._tables)): | ||
| try: | ||
| with self._engine.begin() as conn: | ||
| conn.exec_driver_sql("DROP TABLE IF EXISTS " + _identifier(table)) |
There was a problem hiding this comment.
[P2] Qualify DROP with the schema captured when this object was created. Keeping the original engine does not pin a pooled session's current database. Reproduced on this exact head with Client(database="a", pool_size=1, max_overflow=0): create a file relation, execute the public client.execute("USE b"), then relation.close(). This unqualified DROP runs in b, while the UUID-owned external table remains in a. Close nevertheless removes its reservation and reports closed, so even retrying close cannot reclaim it.
With an independently created same-name b table containing row 42, close deletes that unrelated table as well. Three fresh native pairs reproduce this: no-USE controls preserve b and delete a; all USE cases preserve a and delete b. Capture actual schema+table before CREATE/COMMIT uncertainty and use that frozen qualified identity for relation SQL and DROP; add default-database reuse and foreign-object controls. Full reproduction/evidence is in the review body.
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; parent design acceptance remains open.
What this PR does / why we need it:
Feature and requirements
Add an optional synchronous analytics layer for Parquet/CSV/JSONL exploration through native external tables, atomic DataFrame/Arrow uploads through native IPC LOAD, and bounded Arrow RecordBatch results through a real unbuffered PyMySQL cursor. Share checked scalar mapping, strict transaction settings and explicit resource ownership; PyArrow remains an optional analytics dependency.
Current behavior and review fixes
Fresh validation (2026-10-18)
Python 3.12 / PyArrow 26.0.0 / SQLAlchemy 1.4.54, 2.0.55 and 2.1.5:
Earlier source-build qualification (PyArrow 18/21, 192 analytics cases, dual CN, local moto S3, short soak, constrained memory and six SIGKILL/restart barriers) remains historical evidence in the retained record. Those campaigns were not rerun for this follow-up. Test commands and prerequisites.
CI for this commit: https://github.com/matrixorigin/matrixone/actions/runs/39125091928 (passed: six SDK offline cells and CI Required). Kernel/online jobs follow SDK-only scope. Design re-review, feature acceptance and formal Request Changes remain pending; no approval or merge is claimed.
Limits
Uses ordinary external catalog tables, not private SQL temporary tables or new SQL table functions. Arrow LOAD needs a shared client/CN path; there is no Flight or zero-copy output. Batch size bounds rows, not cell bytes. Network timeouts are inactivity bounds, not total deadlines or server KILL. Source preflight is not an atomic snapshot; crashes/unknown commits require scoped recovery. Local source builds, same-host dual CN and local S3 do not certify a released binary, cloud deployment or multi-day endurance. Integrate #30074's complete connection policy before landing this draft.