Conversation
|
@Jefffrey I'm fixing the failing test. While I'm at it I'm considering adding another change in this PR that uses Display for the set elements rather than Debug. The output now is way too verbose imo. Example from one of the test cases: rather than we would get is that ok to include or should I make a separate PR? |
I think that's a great idea to include in this PR; I recall a recent PR where we saw this debug display too which was a bit ugly: #17732 (comment) |
| let display_string = expr.to_string(); | ||
| assert_eq!(sql_string, "a NOT IN (a, b, NULL)"); | ||
| assert_eq!(display_string, "a@0 NOT IN (SET) ([Literal { value: Utf8(\"a\"), field: Field { name: \"lit\", data_type: Utf8, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} } }, Literal { value: Utf8(\"b\"), field: Field { name: \"lit\", data_type: Utf8, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} } }, Literal { value: Utf8(NULL), field: Field { name: \"lit\", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} } }])"); | ||
| assert_eq!(display_string, "a@0 NOT IN (SET) ([a, b, NULL])"); |
There was a problem hiding this comment.
I've been wondering about the difference in formatting of literals between logical and physical plans. Logical will show Utf8("a") while physical shows just a. Out of scope for what I'm doing here, but would you want the type information in physical as well? For string literals in particular I think quotes would be a useful addition to avoid any possible confusion with column or table names.
There was a problem hiding this comment.
I'll make a separate PR for that since it will probably affect quite a lot of the sql logic test results.
pepijnve
changed the title
Use in InListExpr display formatted outputInListExpr plan display
| let display_string = expr.to_string(); | ||
| assert_eq!(sql_string, "a NOT IN (a, b, NULL)"); | ||
| assert_eq!(display_string, "a@0 NOT IN (SET) ([Literal { value: Utf8(\"a\"), field: Field { name: \"lit\", data_type: Utf8, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} } }, Literal { value: Utf8(\"b\"), field: Field { name: \"lit\", data_type: Utf8, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} } }, Literal { value: Utf8(NULL), field: Field { name: \"lit\", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} } }])"); | ||
| assert_eq!(display_string, "a@0 NOT IN (SET) ([a, b, NULL])"); |
* Remove spurious `Use` in InListExpr display formatted output * Adapt tpch.slt expected results * Reduce verbosity of Display for InListExpr output * Silence clippy warning (cherry picked from commit d273ffb)
3 participants
Which issue does this PR close?
Rationale for this change
Aligns the explain output for
IN (SET)andNOT IN (SET). The presence ofUseis a bit annoying.What changes are included in this PR?
Usefrom the formatted outputAre these changes tested?
Covered by existing tests
Are there any user-facing changes?
The explain output strings change slightly