Repository navigation
Conversation
7027612 to
616c42b
Compare
Reuse the shared schema mock server with per-test resets and count rejected ExecutePlan calls. Render Python UDT class names in schema trees, including nested values, and preserve the generic fallback when metadata is missing. Assert repeated printing performs one schema analysis and tidy test formatting. Convert schema errors within the detached call to avoid a large-error lint. Co-authored-by: Isaac <no-reply@databricks.com>
Escape field names directly into the tree buffer in one pass and write line fragments without allocating temporary formatted strings. Extend the exact output test to preserve Unicode and literal backslashes alongside controls. Co-authored-by: Isaac <no-reply@databricks.com>
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
The fix is right: printSchema now goes through the cached schema() instead of show(0), and the depth and formatting semantics match v4.2.0 StructType.treeString. It also fixes the old nested prefix (" |" → " | |") and the array/map children that used to be inlined. A few things outside the diff:
DataFrame::print_schemaincrates/spark-connect/src/dataframe.rs:1500still doesprintln!("{}", schema), which printssimple_string()(struct<a:int,...>) instead of a tree. It's the same bug for Rust callers. The binding works around it rather than fixing it. Could the core printtree_string(), with the binding reusing it? A follow-up JIRA would also be fine.StructType.treeString(crates/pyspark-rs/src/types.rs:1813) still takesmaxDepth: i32. Sodf.printSchema(2**100)andprintSchema(1.5)work, butdf.schema.treeString(2**100)raises OverflowError andtreeString(1.5)raises TypeError. Reference accepts both. Either share the depth normalization between the two entry points, or keep theOptional[int]contract for both and drop the float/container handling and its tests.- Moving the mock server to module scope also changes the existing schema-cache tests. Please mention why in the description.
| self.append_tree( | ||
| &mut out, | ||
| " |", | ||
| if max_depth > 0 { max_depth } else { i32::MAX }, |
There was a problem hiding this comment.
The doc comment on tree_string_with_depth is stale. Truncation now applies to array/map children as well as structs, and max_depth <= 0 means unlimited, where it used to print only the top level. That's a behavior change in a public API, so it should be documented.
| if remaining <= 0 { | ||
| return; | ||
| } | ||
| let child_prefix = format!("{prefix} |"); |
There was a problem hiding this comment.
This allocates a prefix on every call, including leaf fields that return through _ => {}. Can it move into the Struct/Array/Map arms?
| for f in fields { | ||
| out.push_str(prefix); | ||
| out.push_str("-- "); | ||
| for ch in f.name.chars() { |
There was a problem hiding this comment.
Nit: could this be a small push_escaped_name(out, &f.name) helper, to mirror escape_meta_characters and keep append_tree focused on structure?
| fn printSchema(&self, py: Python<'_>, level: Option<&Bound<'_, PyAny>>) -> PyResult<()> { | ||
| let schema = py.detach(|| self.dataframe.schema().to_pyerr())?; | ||
| // Reference PySpark treats false values and nonpositive depths as unlimited. | ||
| // Positive fractional depths include the partially remaining level as well. |
There was a problem hiding this comment.
Suggest stating the equivalence directly: reference does if level: and then compares > 0 at each level, so a positive fractional depth behaves as ceil(level).
| (None, _PRINT_SCHEMA_FULL), | ||
| (0, _PRINT_SCHEMA_FULL), | ||
| (-1, _PRINT_SCHEMA_FULL), | ||
| (-10, _PRINT_SCHEMA_FULL), |
There was a problem hiding this comment.
Several of these cases cover the same branch: -1/-10, 3/2**100/inf, and ""/[]/{}. Re-checking calls == 1 on every case also re-tests the #120 cache. Can we keep one case per branch and check cache reuse once?
| assert service.execute_calls == 0 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("failure", ["rpc", "no-result", "missing-schema", "invalid-type"]) |
There was a problem hiding this comment.
Retry-after-failure is schema() cache behavior, and the schema-cache tests already cover it for these four failure modes. For printSchema, one case that checks the error propagates and nothing is printed should be enough.
| key_type: Box::new(DataType::String { | ||
| collation: String::new(), | ||
| }), | ||
| value_type: Box::new(udt("UnqualifiedUDT")), |
There was a problem hiding this comment.
Reference can't produce an unqualified python_class: UserDefinedType.fromJson would fail to import it. The same goes for Some("") and Some("example.") at L383. These assertions pin behavior that has no reference to match. Can we drop them and keep the real pyspark.ml.linalg.VectorUDT and None cases?
What changes were proposed in this pull request?
Fix
DataFrame.printSchema()callingshow(0), which executes a zero-row query and emits no schema. Resolve schema metadata instead, render its tree at the requested depth, and call Python'sprintsosys.stdoutcapture works. Preserve reference behavior for default, nonpositive, false, fractional and invalid depth arguments.Match reference tree formatting for scalar type names, nested struct/array/map children and nullability, field-name control-character escaping, and unlimited nonpositive depths. UDTs with Python class metadata use the lowercase class name, such as
vectorudtorcustomtype; unavailable class metadata retains the genericudtfallback. Field names are escaped in one pass and tree lines are written directly to the output buffer. DirectStructType.treeStringand Rust tree consumers are covered alongside the binding.Reuse the existing mock Connect server with a module-scoped process and per-test resets. The tests count ExecutePlan requests and assert repeated printing needs exactly one schema analysis.
Why are the changes needed?
SPARK-60695. Printing a schema should emit schema text without sending ExecutePlan requests.
Does this PR introduce any user-facing change?
Yes.
printSchema()now prints the schema through Python stdout, honors nesting depth, and performs schema analysis instead of query execution. Schema tree text uses reference formatting for nested collection types, scalar type names, and Python UDT class names when available. Repeated calls reuse the schema cache already merged into master.How was this patch tested?
Local review-fix validation at
559cab3(master base617fa148) on Python 3.12 with an optimized release wheel:wasm-udf.5019808also matched 17 exact depth outputs, four invalid-depth errors, and two UDT class names.5019808had the same outcome.No JVM/live Spark server was used. Mock gRPC servers exercise the real client transport in a separate process.
Historical measurements from the preceding implementation, not rerun on the rebased review-fix head:
f87973b1e02fe500e3c4266a8ad3f353244994af. Repeated depth-256 printSchema measured 2.607229 ms → 0.001406 ms (Python reference 0.005496 ms); timed ExecutePlan calls 20 → 0. First-call depth-256 measured 3.174625 → 2.921541 ms. All eight unrelated guard sample ranges overlapped; median changes ranged from -14.6% to +8.0%, without a statistical equivalence claim.The frozen inspection validator did not validate stdout, so those benchmark passes supplement the independent exact-output tests. Raw samples and setup provenance remain in the campaign artifacts. PR120 is now merged into the master base; this PR has no unmerged dependency.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-6).
This pull request and its description were written by Isaac.