andygrove commented on PR #6324: URL: https://github.com/apache/datafusion-comet/pull/6324#issuecomment-5876120710
This is a light fully automated review since there are so many PRs open. 1. `CometArraysZip.getUnsupportedReasons()` (`spark/src/main/scala/org/apache/comet/serde/arrays.scala:848`) still returns only the data type reason. The new duplicate-name reason returned from `getSupportLevel` at `arrays.scala:867` isn't added there, so it won't reach the generated compatibility guide. `CometCreateNamedStruct.getUnsupportedReasons()` in `structs.scala`, the exact pattern this PR says it's following, does include its `duplicateNamesReason`. Could the new reason be added to `getUnsupportedReasons()` here too? 2. `spark/src/test/resources/sql-tests/expressions/array/arrays_zip.sql` already defines a `test_arrays_zip(a array<int>, b array<int>)` table with data loaded, and the SQL file test framework has a `query expect_fallback(<reason>)` directive built for exactly this case (see `routing_arrays_opt_in.sql:59` for a similar array data-type fallback case). The new Scala test at `CometExpressionSuite.scala:3927` uses `checkSparkAnswer`, which by its own doc comment does not check whether Comet accelerated the query. So the test would still pass if the query fell back for some other reason and never reached the new guard. Would `select arrays_zip(a, a) FROM test_arrays_zip` as an `expect_fallback` case in the existing `.sql` file be a better fit here? -- 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]
