andygrove commented on code in PR #5798: URL: https://github.com/apache/datafusion-comet/pull/5798#discussion_r3973626312
########## spark/src/test/resources/sql-tests/expressions/array/get_array_item_ansi.sql: ########## @@ -25,34 +25,36 @@ -- ============================================================================ statement -CREATE TABLE ansi_array_oob(arr array<int>) USING parquet +CREATE TABLE ansi_array_oob(arr array<int>, positive_idx int, negative_idx int) USING parquet statement -INSERT INTO ansi_array_oob VALUES (array(1, 2, 3)) +INSERT INTO ansi_array_oob VALUES (array(1, 2, 3), 5, -1) + +-- Valid boundary indices must run natively as well as match Spark. +query +SELECT arr[0], arr[2] FROM ansi_array_oob Review Comment: Adding these valid boundary queries is the right instinct given `expect_error` skips the operator check. Since you are pinning boundaries anyway, would it be worth pairing them with the first invalid index too? `arr[3]` is the one that would catch an off-by-one in `zero_based_index`, and `arr[10]` would sail straight past it. I tried it locally and it behaves correctly today, so this is purely locking in what already works. ########## spark/src/test/resources/sql-tests/expressions/array/get_array_item_ansi.sql: ########## @@ -25,34 +25,36 @@ -- ============================================================================ statement -CREATE TABLE ansi_array_oob(arr array<int>) USING parquet +CREATE TABLE ansi_array_oob(arr array<int>, positive_idx int, negative_idx int) USING parquet statement -INSERT INTO ansi_array_oob VALUES (array(1, 2, 3)) +INSERT INTO ansi_array_oob VALUES (array(1, 2, 3), 5, -1) + +-- Valid boundary indices must run natively as well as match Spark. +query +SELECT arr[0], arr[2] FROM ansi_array_oob -- ============================================================================ -- Array index out of bounds (positive index) --- Spark throws: [INVALID_ARRAY_INDEX] The index X is out of bounds --- Comet throws: Index out of bounds for array --- See https://github.com/apache/datafusion-comet/issues/3375 +-- Spark and Comet throw INVALID_ARRAY_INDEX in ANSI mode. -- ============================================================================ -- index beyond array length should throw (0-based indexing) -query ignore(https://github.com/apache/datafusion-comet/issues/3375) +query expect_error(INVALID_ARRAY_INDEX) Review Comment: Nice catch on `SimplifyExtractValueOps`. I checked `ComplexTypes.scala` on 3.4 through 4.1 and the rule really does fold an out-of-range `CreateArray` lookup to a null literal with no ANSI guard, so the column index is the only way these cases can throw at all. On the pattern itself though, `expect_error` is a substring match on the message, and `INVALID_ARRAY_INDEX` happens to be a prefix of `INVALID_ARRAY_INDEX_IN_ELEMENT_AT`. That means all four of these would still pass if the bracket path ever started raising the `element_at` error class, which is exactly the confusion this file exists to rule out. Could we use `expect_error([INVALID_ARRAY_INDEX])` instead? Both engines render the bracketed form, so that pins the class exactly. Worth doing the same in `element_at_ansi.sql` for consistency. ########## spark/src/test/resources/sql-tests/expressions/array/element_at_ansi.sql: ########## @@ -31,46 +31,47 @@ CREATE TABLE ansi_element_at_oob(arr array<int>) USING parquet statement INSERT INTO ansi_element_at_oob VALUES (array(1, 2, 3)) +-- Valid positive and negative boundary indices must run natively and match Spark. +query +SELECT element_at(arr, 1), element_at(arr, 3), element_at(arr, -1), element_at(arr, -3) +FROM ansi_element_at_oob + -- ============================================================================ -- element_at index out of bounds (positive index) --- Spark throws: [INVALID_ARRAY_INDEX_IN_ELEMENT_AT] ... --- Comet throws: Index out of bounds for array --- See https://github.com/apache/datafusion-comet/issues/3375 +-- Spark and Comet throw INVALID_ARRAY_INDEX_IN_ELEMENT_AT in ANSI mode. -- ============================================================================ -- index beyond array length should throw (1-based indexing) -query ignore(https://github.com/apache/datafusion-comet/issues/3375) +query expect_error(INVALID_ARRAY_INDEX_IN_ELEMENT_AT) SELECT element_at(arr, 10) FROM ansi_element_at_oob -- literal array with out of bounds access -query ignore(https://github.com/apache/datafusion-comet/issues/3375) +query expect_error(INVALID_ARRAY_INDEX_IN_ELEMENT_AT) SELECT element_at(array(1, 2, 3), 5) Review Comment: These three literal-array cases (this one, line 63 and line 75) have no `FROM`, so they plan as `CometProject` over `CometSparkRowToColumnar` over `Scan OneRowRelation`. That only reaches Comet because `CometTestBase` sets `spark.comet.sparkToColumnar.enabled` to true, and the product default for that config is false. Since `expect_error` does not assert native execution the way `query` does, if that test base setting ever changed these three would quietly become Spark vs Spark and keep passing. Would you mind adding `FROM ansi_element_at_oob` to them? The table has a single row so the expected error is unchanged, and it puts them over a real Comet scan like everything else in the file. It also matches what you already did on the `get_array_item` side. ########## spark/src/test/resources/sql-tests/expressions/array/element_at_ansi.sql: ########## @@ -31,46 +31,47 @@ CREATE TABLE ansi_element_at_oob(arr array<int>) USING parquet statement INSERT INTO ansi_element_at_oob VALUES (array(1, 2, 3)) +-- Valid positive and negative boundary indices must run natively and match Spark. +query +SELECT element_at(arr, 1), element_at(arr, 3), element_at(arr, -1), element_at(arr, -3) Review Comment: Same thought as on the `get_array_item` file. Would it be worth pairing the valid boundaries with the first invalid index rather than only 10 and -10? `element_at(arr, 4)` and `element_at(arr, -4)` are where an off-by-one in `one_based_index` in `native/spark-expr/src/array_funcs/list_extract.rs` would actually surface, and the far out of range values would not catch it. Both behave correctly today, so this is just locking in what already works. One more line while you are in here would be useful. `SELECT element_at(arr, CAST(NULL AS INT)) FROM ansi_element_at_oob` returns NULL rather than throwing even under ANSI, and nothing in this file guards that branch. -- 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]
