Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,47 +31,58 @@ 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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

FROM ansi_element_at_oob

-- A NULL index returns NULL even in ANSI mode.
query
SELECT element_at(arr, CAST(NULL AS INT)) 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, 4) FROM ansi_element_at_oob

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)
SELECT element_at(array(1, 2, 3), 5)
query expect_error([INVALID_ARRAY_INDEX_IN_ELEMENT_AT])
SELECT element_at(array(1, 2, 3), 5) FROM ansi_element_at_oob

-- ============================================================================
-- element_at with index 0 (invalid)
-- Spark throws: [INVALID_INDEX_OF_ZERO] The index 0 is invalid
-- Comet throws: different error message
-- See https://github.com/apache/datafusion-comet/issues/3375
-- Spark and Comet throw INVALID_INDEX_OF_ZERO.
-- ============================================================================

-- index 0 is not valid for element_at (1-based indexing)
query ignore(https://github.com/apache/datafusion-comet/issues/3375)
query expect_error([INVALID_INDEX_OF_ZERO])
SELECT element_at(arr, 0) FROM ansi_element_at_oob

-- literal with index 0
query ignore(https://github.com/apache/datafusion-comet/issues/3375)
SELECT element_at(array(1, 2, 3), 0)
query expect_error([INVALID_INDEX_OF_ZERO])
SELECT element_at(array(1, 2, 3), 0) FROM ansi_element_at_oob

-- ============================================================================
-- element_at index out of bounds (negative index beyond array)
-- ============================================================================

-- negative index beyond array size should throw
query ignore(https://github.com/apache/datafusion-comet/issues/3375)
query expect_error([INVALID_ARRAY_INDEX_IN_ELEMENT_AT])
SELECT element_at(arr, -4) FROM ansi_element_at_oob

query expect_error([INVALID_ARRAY_INDEX_IN_ELEMENT_AT])
SELECT element_at(arr, -10) FROM ansi_element_at_oob

-- literal with negative out of bounds
query ignore(https://github.com/apache/datafusion-comet/issues/3375)
SELECT element_at(array(1, 2, 3), -5)
query expect_error([INVALID_ARRAY_INDEX_IN_ELEMENT_AT])
SELECT element_at(array(1, 2, 3), -5) FROM ansi_element_at_oob

-- ============================================================================
-- ANSI short-circuit over a NULL array
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,34 +25,39 @@
-- ============================================================================

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok , I will address it


-- ============================================================================
-- 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])
SELECT arr[3] FROM ansi_array_oob

query expect_error([INVALID_ARRAY_INDEX])
SELECT arr[10] FROM ansi_array_oob

-- literal array with out of bounds access
query ignore(https://github.com/apache/datafusion-comet/issues/3375)
SELECT array(1, 2, 3)[5]
-- Use a column index so SimplifyExtractValueOps cannot replace the lookup with NULL.
query expect_error([INVALID_ARRAY_INDEX])
SELECT array(1, 2, 3)[positive_idx] FROM ansi_array_oob

-- ============================================================================
-- Array index out of bounds (negative index)
-- ============================================================================

-- negative index should throw
query ignore(https://github.com/apache/datafusion-comet/issues/3375)
query expect_error([INVALID_ARRAY_INDEX])
SELECT arr[-1] FROM ansi_array_oob

-- literal with negative index
query ignore(https://github.com/apache/datafusion-comet/issues/3375)
SELECT array(1, 2, 3)[-1]
-- literal array with a negative column index
query expect_error([INVALID_ARRAY_INDEX])
SELECT array(1, 2, 3)[negative_idx] FROM ansi_array_oob
Loading