HyukjinKwon commented on PR #50: URL: https://github.com/apache/spark-connect-rust/pull/50#issuecomment-5393949580
Thanks for the thorough re-review — this was exactly the right set of things to catch. Addressed point by point (all pushed to this branch): **Stack is now real.** Rebased so this branch is the actual base of #51; #51's base was re-synced to this tip and now shows only the Python diff (no duplicated core). **crates.io names.** Renamed the publishable crates to `apache-spark-connect` / `apache-spark-connect-core` / `apache-spark-connect-proto`, with `[lib] name` preserving `spark_connect*` so imports and the wheel build are unchanged; `release.yml` publishes in that dependency order. Good call on the outgoing `spark-connect-rs 0.0.2` — a final `0.0.3` marked deprecated pointing here is worth doing as a separate release step (not baked into this PR). **Silent plan-builder bugs — all fixed, each with a regression test:** - `dropna(how)` now derives `min_non_nulls` from `how` (`all`→1, `any`→unset, explicit thresh overrides). - `hint(parameters)` serializes its parameters. - `replace()` builds string/bool literals (no longer no-ops on non-numeric). - explicit-value `pivot` serializes `pivot_values` (added the API to pass them). - `fillna` accepts double/string/bool. **The wider class.** Took your "how many more of the same shape" seriously and swept the whole plan builder — found and fixed more: `createTempView`/`createOrReplaceTempView`/global variants were silent no-ops (now issue a real `CreateDataFrameViewCommand`), `explain()`/`inputFiles()` were passthrough/empty (now hit `AnalyzePlan`), `LocalRelation` dropped the explicit schema, the caching family (`cache`/`persist`/`unpersist`/`storageLevel`/`isCached`) were `Hint`/hardcoded fakes (now real `AnalyzePlan` persist ops, returning `Result`), and `checkpoint`/`localCheckpoint` now issue a real `CheckpointCommand`. `audit_no_stubs.sh` was broadened and made case-insensitive (it now catches `in the future`/`for now`/`placeholder`/`deferred`) and passes. **Retries/reattach — now wired (were dead).** `RetryPolicy` drives backoff on the RPCs, and a `ReattachableResponseStream` consumes the stream and resumes via `ReattachExecute` from the last `response_id`; `collect`/`collect_record_batches`/`execute_command` route through it. **Governance.** Kept `issues: false` for now at the repo owner's request, but you're right it belongs on dev@ — happy to move that discussion there and revert if the list prefers. Reconciled the README so `4.2.0` reads as a Spark-release-tracking version rather than a maturity claim. Please file JIRAs if you'd prefer per-bug tracking — otherwise these are all in this branch now. (Note: this branch also now carries an experimental Rust-UDF-via-WASM commit.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
