HyukjinKwon commented on PR #53:
URL:
https://github.com/apache/spark-connect-rust/pull/53#issuecomment-5408436448
Thanks for the review. All six findings are addressed on the current head
(`84c7599`). The review ran against `8e95ff8`, so two of the six were already
resolved by the snippet-accuracy pass I pushed just before it (`798147e`); the
rest are fixed now.
**1. `docs.yml` deployment guard** - fixed. Now a conjunction that also pins
the canonical repo:
`if: github.repository == 'apache/spark-connect-rust' && github.ref ==
'refs/heads/master'` (covers push and `workflow_dispatch`; forks and non-master
refs never deploy).
**2. `gen_parity_ledger.py` status file** - fixed. The docstring no longer
references a `status.csv`; it now states that `status`/`notes` live in the
columns of `dev/parity/inventory.csv` and are read back and preserved on
regeneration.
**3. Async API shape (quickstart/configuration/dataframes/examples/sql)** -
already fixed in `798147e`, and re-verified: no `#[tokio::main]`, `.await`,
two-arg `range`, or `show(None)` remain. I compiled every Rust snippet in the
docs against the crate (complete programs as-is, fragments in a scaffold):
**all 15 code bins compile, 0 failures**, and I ran several against a live
Spark Connect server to confirm correct results.
**4. `types.md` enum/struct shapes** - already fixed in `798147e`. The table
and schema snippets use the real variants: `String { collation }`, `Array {
element_type, contains_null }`, `Map { key_type, value_type,
value_contains_null }`, `Struct { fields }`, and `StructField { name,
data_type, nullable, metadata }`. Compiles.
**5. Connection-URL delimiter** - fixed. The code examples and the
troubleshooting example now use `sc://host:15002/;...` (the table already did);
this matches `ChannelBuilder::parse` (`_splitparams` begins after the last `/`).
**6. SQL "parameter binding" claim** - fixed. `SparkSession::sql` takes only
a query string, so the section is retitled and now recommends the **typed
DataFrame API** (`.filter(f::col(..).gt(lit(..)))`) as the injection-safe path
for caller-controlled values, with `format!` interpolation demoted to a warning
("trusted input only, NOT injection-safe").
One extra issue I found while running the examples (beyond this review):
`functions::when(..).otherwise(..)` silently drops the `else` branch (returns
NULL) because `functions::when` builds a plain function call rather than a
`CaseWhen`; the CASE/WHEN builder is `spark_connect::column::when`. The docs
now use `column::when`. That looks like a crate-API gap worth its own follow-up
(either re-export/redirect `when`, or make `functions::when` build a
`CaseWhen`).
--
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]
