huangxiaopingRD commented on PR #56587:
URL: https://github.com/apache/spark/pull/56587#issuecomment-4758840300
> **0 blocking, 2 non-blocking, 0 nits.** A correct, minimal bug fix. The
root cause diagnosis is right: `properties` was collapsed into a single
`String` by the early `.mkString("[", ",", "]")`, so the later
`properties.mkString(...)` iterated that string character-by-character. Keeping
it a `List[String]` until the final `mkString` is the right fix, and it matches
the existing `getTablePartitionDetails` pattern in the same file.
>
> ### Correctness (1)
> * `ShowTablesSuiteBase.scala:424`: no regression test pins this fix. The
shared `replace()` helper masks the value wholesale (`case s"Table
Properties:$_" => "Table Properties: <table properties>"`), so every `show
table extended` assertion compares the masked string — the pre-fix garbage
output would have masked to the same `<table properties>` and passed, and a
future regression would stay invisible too. Please add a v2 `ShowTablesSuite`
case that creates a table with `TBLPROPERTIES ('p1'='v1','p2'='v2')` and
asserts the unmasked `Table Properties: [p1=v1, p2=v2]` line (or stop masking
Table Properties for the v2 suite).
>
> ### Suggestions (1)
> * `ShowTablesExtendedExec.scala:105` (pre-existing, out of scope): the `if
(!table.properties().isEmpty)` guard tests the raw properties while the
displayed list is filtered, so an all-reserved-properties table prints `Table
Properties: []`.
>
> ## PR description suggestions
> * Document: "How was this patch tested?" is empty — please state whether a
test was added (and if not, why).
@MaxGekk @uros-b Thank you for your review. The code has been updated.
Please review it again.
--
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]
