xiedeyantu commented on PR #21513: URL: https://github.com/apache/datafusion/pull/21513#issuecomment-4224593751
> Nice change, this looks correct to me. One request before approving: could you add a test that exercises the second code path you modified? > > The current test `SELECT [1, 2, 3, 4, 5]` only hits the `make_array` scalar function branch (around line 604). The other hunk you changed (around line 615, the `ScalarValue::List` branch) isn't covered, since the literal stays as a `make_array` call through the roundtrip and never reaches that code. > > A small unit test that builds an `Expr::Literal(ScalarValue::List(...))` directly and calls `expr_to_sql` under `PostgreSqlDialect`, asserting the output is `ARRAY[1, 2, 3]`, would pin that line down. A nested case like `[[1, 2], [3, 4]]` → `ARRAY[ARRAY[1, 2], ARRAY[3, 4]]` would also be a nice sanity check that recursion through the hook works as expected. > > Otherwise LGTM. I have added 2 cases based on your comments. Please take a look. -- 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]
