andygrove commented on code in PR #5614:
URL: https://github.com/apache/datafusion-comet/pull/5614#discussion_r3941143051
##########
spark/src/main/spark-3.4/org/apache/spark/sql/comet/shims/ShimSparkErrorConverter.scala:
##########
@@ -158,10 +158,32 @@ trait ShimSparkErrorConverter {
Some(QueryExecutionErrors.exceedMapSizeLimitError(params("size").toString.toInt))
case "CollectionSizeLimitExceeded" =>
- // createArrayWithElementsExceedLimitError takes (count: Any) in Spark
3.4
+ // createArrayWithElementsExceedLimitError takes (count: Any) in Spark
3.4; pass the
+ // decimal string through since the reported length can exceed Long
range.
Some(
QueryExecutionErrors.createArrayWithElementsExceedLimitError(
- params("numElements").toString.toLong))
+ params("numElements").toString))
+
+ case "SequenceIllegalBoundaries" =>
+ // Spark 3.x codegen throws a plain IllegalArgumentException for
sequence boundaries.
+ Some(
+ new IllegalArgumentException(
+ s"Illegal sequence boundaries: ${params("start")} to
${params("stop")} " +
+ s"by ${params("step")}"))
+
+ case "SequenceBatchTooLarge" =>
Review Comment:
This message now exists in four places, `error.rs` plus all three shims, and
unlike `SequenceIllegalBoundaries` it has no version-specific behaviour to
justify living in the shims at all. Would a shared constant work, with each
shim interpolating `params("totalElements")` into it? The same question applies
to the `case "Internal"` arm just below, which is character-identical in all
three files. Four copies of one English sentence is the kind of thing where a
later wording fix lands in three places and misses the fourth.
##########
docs/source/user-guide/latest/expressions.md:
##########
@@ -168,7 +168,7 @@ The tables below list every Spark built-in expression with
its current status.
| `element_at` | ✅ | Native | |
| `flatten` | ✅ | Native | Binary/struct/map elements fall back |
| `get` | ✅ | — | |
-| `sequence` | ✅ | Codegen dispatch | |
+| `sequence` | ✅ | Hybrid | Integral types run natively; date/timestamp
sequences use codegen dispatch |
Review Comment:
This note describes the native versus dispatcher split, which a user cannot
observe, and leaves out the per-batch ceiling, which is the one thing they can,
since it is a query Spark runs that Comet fails. Could it mention that as well,
something like "very large per-row sequences may exceed Comet's per-batch
limit, lower `spark.comet.batchSize`"? The audit entry covers it well, but that
is not where somebody who has just hit the error will be looking.
##########
spark/src/test/resources/sql-tests/expressions/array/sequence.sql:
##########
@@ -15,17 +15,175 @@
-- specific language governing permissions and limitations
-- under the License.
--- Routes sequence through the codegen dispatcher so behavior matches Spark
exactly.
+-- sequence(start, stop[, step]) for integral element types runs on the native
kernel
+-- (https://github.com/apache/datafusion-comet/issues/5349). Date and
timestamp sequences
+-- stay on the JVM codegen dispatcher and are exercised at the bottom of this
file.
statement
-CREATE TABLE test_sequence(a int, b int) USING parquet
+CREATE TABLE test_sequence(
+ b_start tinyint, b_stop tinyint, b_step tinyint,
+ s_start smallint, s_stop smallint, s_step smallint,
+ i_start int, i_stop int, i_step int,
+ l_start bigint, l_stop bigint, l_step bigint)
+USING parquet
+-- Row 2 descends, row 3 has start == stop, rows 4-6 carry NULLs in each
argument position.
statement
-INSERT INTO test_sequence VALUES (1, 5), (5, 1), (3, 3), (NULL, 5)
+INSERT INTO test_sequence VALUES
+ (1Y, 5Y, 1Y, 1S, 5S, 1S, 1, 10, 3, 1L, 5L, 2L),
+ (-3Y, -1Y, 1Y, 100S, 90S, -2S, 20, 2, -6, 9223372036854775802L,
9223372036854775807L, 1L),
+ (0Y, 0Y, 0Y, -5S, -5S, 0S, 7, 7, 0, -9223372036854775808L,
-9223372036854775800L, 3L),
+ (NULL, 5Y, 1Y, NULL, 5S, 1S, NULL, 10, 1, NULL, 5L, 1L),
+ (1Y, NULL, 1Y, 1S, NULL, 1S, 1, NULL, 1, 1L, NULL, 1L),
+ (1Y, 5Y, NULL, 1S, 5S, NULL, 1, 10, NULL, 1L, 5L, NULL)
+
+-- ============================================================================
+-- Explicit step, all four integral types
+-- ============================================================================
+
+query
+SELECT sequence(i_start, i_stop, i_step) FROM test_sequence
+
+query
+SELECT sequence(l_start, l_stop, l_step) FROM test_sequence
+
+-- Column step for the narrow integral types exercises the Byte/Short
monomorphizations
+-- of the native kernel, not just the literal-step shape.
+query
+SELECT sequence(b_start, b_stop, b_step) FROM test_sequence
+
+query
+SELECT sequence(s_start, s_stop, s_step) FROM test_sequence
+
+-- ============================================================================
+-- Default step: per-row start <= stop ? 1 : -1, both directions in one column
+-- ============================================================================
+
+query
+SELECT sequence(b_start, b_stop), sequence(s_start, s_stop) FROM test_sequence
+
+query
+SELECT sequence(i_start, i_stop), sequence(l_start, l_stop) FROM test_sequence
+
+-- ============================================================================
+-- Literal and mixed literal/column arguments
+-- ============================================================================
query
-SELECT a, b, sequence(a, b) FROM test_sequence
+SELECT sequence(1, 10), sequence(10, 1), sequence(5, 5), sequence(5, 5, 0)
--- literal arguments with step
query
SELECT sequence(1, 5), sequence(5, 1, -1), sequence(1, 10, 2)
+
+query
+SELECT sequence(1L, 9L, 2L), sequence(-128Y, -120Y), sequence(32760S, 32767S)
+
+-- On row 2 the source row is (i_start=20, i_stop=2, i_step=-6), so the
literal-step column
+-- asks for sequence(1, 2, 2) = [1] while the default-step column asks for
sequence(20, 25)
+-- = [20, 21, 22, 23, 24, 25]. The two columns disagreeing in direction on the
same row is
+-- intentional coverage, not an oversight.
+query
+SELECT sequence(1, i_stop, 2), sequence(i_start, 25) FROM test_sequence WHERE
i_start IS NOT NULL AND i_stop IS NOT NULL
+
+query
+SELECT sequence(CAST(NULL AS int), 5), sequence(1, CAST(NULL AS int)),
sequence(1, 5, CAST(NULL AS int))
+
+-- Integer.MIN_VALUE/MAX_VALUE bounds for int, and a sequence spanning zero
+query
+SELECT sequence(2147483642, 2147483647), sequence(-2147483648, -2147483643),
sequence(-3, 3, 3)
+
+-- ============================================================================
+-- sequence feeding explode, the common date-spine shape (with integers)
+-- ============================================================================
+
+query
+SELECT i_start, x FROM test_sequence LATERAL VIEW explode(sequence(i_start,
i_stop)) AS x WHERE i_start IS NOT NULL AND i_stop IS NOT NULL
+
+-- ============================================================================
+-- Error paths: step direction contradicts bounds, or zero step with start !=
stop
+-- ============================================================================
+
+query expect_error(Illegal sequence boundaries: 1 to 5 by -1)
+SELECT sequence(1, 5, -1)
+
+query expect_error(Illegal sequence boundaries: 10 to 2 by 3)
+SELECT sequence(10, 2, 3) FROM test_sequence LIMIT 1
+
+query expect_error(Illegal sequence boundaries: 1 to 5 by 0)
+SELECT sequence(1, 5, 0)
+
+-- ============================================================================
+-- Error paths: length exceeds MAX_ROUNDED_ARRAY_LENGTH
+-- ============================================================================
+
+query expect_error(the array size limit 2147483632)
+SELECT sequence(0L, 4294967296L, 1L)
+
+-- Math.addExact overflow inside Spark's sequenceLength: reported count is 2^63
+query expect_error(9223372036854775808)
+SELECT sequence(0L, 9223372036854775807L, 1L)
+
+-- Long.MinValue / -1 special case: reported count is 2^63 + 1
+query expect_error(9223372036854775809)
+SELECT sequence(0L, -9223372036854775808L, -1L)
+
+-- delta overflows long but the exact length is tiny: Spark reaches an
internal error
+query expect_error(Unreachable code reached)
+SELECT sequence(-9223372036854775808L, 9223372036854775807L,
9223372036854775807L)
+
+-- ============================================================================
+-- Full narrow-type range: writes at the byte/short boundary. Spark's kernel
+-- accumulates with the element type's `Numeric`, wrapping at 8 and 16 bits;
+-- ours accumulates in i64 and truncates on the way out. The two agree because
+-- every element is inside range, but this locks in the boundary values.
+-- ============================================================================
+
+query
+SELECT sequence(-128Y, 127Y), sequence(-32768S, 32767S)
+
+-- ============================================================================
+-- Int32 boundary product: index * step overflows int, exercising the Int32
+-- monomorphization at the extreme.
+-- ============================================================================
+
+query
+SELECT sequence(-2147483648, 2147483647, 1073741824)
+
+-- ============================================================================
+-- Null short-circuit under a nested sequence: Spark's codegen returns NULL
+-- without evaluating the inner argument, so the inner `sequence(1, 5, -1)`
+-- must not fire on the NULL row. Non-leaf argument shapes stay on the JVM
+-- codegen dispatcher for this reason
+--
(https://github.com/apache/datafusion-comet/pull/5614#discussion_r3910237757).
Review Comment:
Could the `#discussion_r3910237757` link come out of this comment? The two
sentences before it already say why non-leaf argument shapes stay on the
dispatcher, which is the part a future reader needs. A pointer into a review
thread records how the code came to be rather than what it does, and it will
not survive the next change to this reasoning. The `#5349` link at the top of
the file is the durable kind and is worth keeping.
##########
native/spark-expr/src/array_funcs/sequence.rs:
##########
@@ -0,0 +1,393 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Spark-compatible sequence(start, stop[, step]) for integral element types.
+//
+// Mirrors the code Spark's whole-stage codegen emits for `Sequence`
+//
(`sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala`,
+// identical from 3.4.3 through 4.1.1): the boundary check and
`Sequence.sequenceLength` decide
+// per row how many elements to generate, then elements are `start + step *
i`. Unlike the JVM
+// path, which allocates two `long[]` per row and copies every element three
times, this kernel
+// reserves the Arrow child buffer once for the whole batch and writes each
element exactly once.
+//
+// Date/timestamp sequences are not handled here; the Scala serde only routes
IntegralType
+// sequences to this function.
+
+use std::sync::Arc;
+
+use arrow::array::{Array, ArrayRef, ListArray, NullBufferBuilder,
PrimitiveArray};
+use arrow::buffer::{OffsetBuffer, ScalarBuffer};
+use arrow::datatypes::{
+ ArrowPrimitiveType, DataType, FieldRef, Int16Type, Int32Type, Int64Type,
Int8Type,
+};
+use datafusion::common::cast::as_primitive_array;
+use datafusion::common::{exec_err, DataFusionError, Result, ScalarValue};
+use datafusion::logical_expr::ColumnarValue;
+
+use crate::SparkError;
+
+/// Spark's ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH (Integer.MAX_VALUE - 15).
+const MAX_ROUNDED_ARRAY_LENGTH: i128 = (i32::MAX - 15) as i128;
+
+pub fn spark_sequence(args: &[ColumnarValue], data_type: &DataType) ->
Result<ColumnarValue> {
+ let child_field = match data_type {
+ DataType::List(field) => Arc::clone(field),
+ other => return exec_err!("spark_sequence expects a List return type,
got {other:?}"),
+ };
+ if args.len() != 2 && args.len() != 3 {
+ return exec_err!(
+ "spark_sequence expects 2 or 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ let all_scalar = args
+ .iter()
+ .all(|arg| matches!(arg, ColumnarValue::Scalar(_)));
+ let arrays = ColumnarValue::values_to_arrays(args)?;
+ let step = arrays.get(2);
+
+ let result = match child_field.data_type() {
+ DataType::Int8 => {
+ sequence_integral::<Int8Type>(&arrays[0], &arrays[1], step,
child_field, |v| v as i8)
+ }
+ DataType::Int16 => {
+ sequence_integral::<Int16Type>(&arrays[0], &arrays[1], step,
child_field, |v| v as i16)
+ }
+ DataType::Int32 => {
+ sequence_integral::<Int32Type>(&arrays[0], &arrays[1], step,
child_field, |v| v as i32)
+ }
+ DataType::Int64 => {
+ sequence_integral::<Int64Type>(&arrays[0], &arrays[1], step,
child_field, |v| v)
+ }
+ other => exec_err!("spark_sequence does not support element type
{other:?}"),
+ }?;
+
+ if all_scalar {
+ Ok(ColumnarValue::Scalar(ScalarValue::try_from_array(
+ &result, 0,
+ )?))
+ } else {
+ Ok(ColumnarValue::Array(result))
+ }
+}
+
+fn sequence_integral<T: ArrowPrimitiveType>(
+ start: &ArrayRef,
+ stop: &ArrayRef,
+ step: Option<&ArrayRef>,
+ child_field: FieldRef,
+ from_i64: impl Fn(i64) -> T::Native,
+) -> Result<ArrayRef>
+where
+ T::Native: Into<i64>,
+{
+ let start = as_primitive_array::<T>(start)?;
+ let stop = as_primitive_array::<T>(stop)?;
+ let step = step.map(|arr| as_primitive_array::<T>(arr)).transpose()?;
+ let num_rows = start.len();
+
+ let row_is_null = |row: usize| {
+ start.is_null(row) || stop.is_null(row) || step.is_some_and(|arr|
arr.is_null(row))
+ };
+ // With no explicit step, Spark uses `start <= stop ? 1 : -1` per row, so
the direction
+ // always matches the bounds and the boundary check below cannot fail.
+ let row_step = |row: usize, start: i64, stop: i64| -> i64 {
+ match step {
+ Some(arr) => arr.value(row).into(),
+ None => {
+ if start <= stop {
+ 1
+ } else {
+ -1
+ }
+ }
+ }
+ };
+
+ // First pass: compute per-row lengths so the child buffer can be reserved
once for the
+ // whole batch. Valid rows always produce at least one element, so length
0 marks a null row.
+ let mut lengths: Vec<usize> = Vec::with_capacity(num_rows);
+ let mut total: usize = 0;
+ for row in 0..num_rows {
+ if row_is_null(row) {
+ lengths.push(0);
+ continue;
+ }
+ let s: i64 = start.value(row).into();
+ let e: i64 = stop.value(row).into();
+ let len = sequence_length(s, e, row_step(row, s, e))?;
+ total += len;
+ lengths.push(len);
+ }
+ // Comet-specific ceiling: the sum of every row's length in one Arrow
batch must fit in
+ // the i32 offset buffer. Spark has no equivalent guard because it stores
each row as its
+ // own `long[]`, so the user may hit this on a query Spark itself would
run. Report it via
+ // a dedicated error that names `spark.comet.batchSize` as the actionable
knob rather than
+ // Spark's per-array size limit.
+ if total > i32::MAX as usize {
Review Comment:
The `try_reserve_exact` change does what I asked for, thank you. I think
there is still a gap underneath it though.
This guard is on element count rather than bytes, so for `bigint` it only
fires at around 17 GB, and the `Vec` comes from the global allocator rather
than the DataFusion `MemoryPool`. That means the allocation is not counted
against `spark.comet.memory*`, cannot be spilled, and applies no back-pressure.
At the default batch size I measured 50000 elements per row allocating 3.3
GB in a single reservation and completing fine, with peak process RSS 2852 MB
above baseline against Spark's 1076 MB for the same query. On a Linux executor
with overcommit the OOM killer arrives well before `try_reserve_exact` gets a
chance to return `Err`, so the graceful path is the one a user is least likely
to reach.
Could the ceiling be a byte budget as well as an element count, sized off
the batch memory budget rather than `i32::MAX`? That would make
`SequenceBatchTooLarge` fire while the executor is still healthy, which is the
point at which its actionable message is worth something. This is also the
peak-memory question from the earlier round, which I do not think has been
answered with a measurement yet.
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -954,4 +954,61 @@ object CometArraySort extends
CometCodegenDispatch[ArraySort]
object CometZipWith extends CometCodegenDispatch[ZipWith]
-object CometSequence extends CometCodegenDispatch[Sequence]
+object CometSequence extends CometExpressionSerde[Sequence] with
CodegenDispatchFallback {
+
+ private val temporalUnsupportedReason =
+ "date and timestamp element types run through the JVM codegen dispatcher"
+
+ private val unsafeArgUnsupportedReason =
+ "sequence arguments must be literals or column references; other shapes
run through the " +
+ "JVM codegen dispatcher to preserve Spark's per-row null short-circuit"
+
+ override def getSupportLevel(expr: Sequence): SupportLevel =
expr.start.dataType match {
+ case ByteType | ShortType | IntegerType | LongType =>
+ // Spark's codegen for `Sequence` short-circuits per row: any null
argument returns null
+ // without evaluating the rest. DataFusion evaluates each scalar-UDF
argument over the
+ // whole batch before calling the outer kernel, so a sub-expression with
side effects
+ // (a nested call, a `CASE WHEN`, even a zero-arg UDF like `boom()`)
could fire on rows
+ // Spark's null check would have discarded. A tree-shape "no children"
test is not
+ // enough — a zero-arg UDF has empty children but still executes. Only
literals and
+ // column references are safe to lower natively; anything else falls
back to the
+ // codegen dispatcher, which keeps the whole tree inside Spark's guarded
evaluation.
+ if (argsAreLiteralsOrRefs(expr)) Compatible()
+ else Unsupported(Some(unsafeArgUnsupportedReason))
+ case DateType | TimestampType | TimestampNTZType =>
+ // Temporal sequences step through timezone/DST/legacy-calendar
arithmetic
+ // (https://github.com/apache/datafusion-comet/issues/5349), so they
stay on the JVM
+ // codegen dispatcher.
+ Unsupported(Some(temporalUnsupportedReason))
+ case other =>
+ Unsupported(Some(s"sequence with element type $other is not supported
natively"))
+ }
+
+ private def argsAreLiteralsOrRefs(expr: Sequence): Boolean = {
Review Comment:
The gate itself is the right conservative call and I am not arguing with it.
It is worth being explicit about how narrow the resulting fast path is though.
The native kernel only engages when both endpoints already exist as columns or
literals, so the idiomatic spine `sequence(x, x + n)` stays on the dispatcher,
and so does anything behind a coercion `CAST`. Your own benchmark is the
evidence, since it needed `c_stop_5` and friends materialised as stored columns
before any integral case went native.
I also tried the workaround a user would reach for first, and it does not
work. `FROM (SELECT c_start, c_start + 364 AS c_stop FROM p)` gets folded
straight back by `CollapseProject`, and the explain still reports `JVM codegen
dispatcher: sequence`. So there is no way to opt in short of rewriting the
table.
Two things would help. Could the audit entry spell out which shapes reach
the native path, since "leaf arguments only" is not something a user can map
onto their own SQL? And separately, is a safe widening worth considering later,
accepting an argument subtree that provably cannot throw and preserves nulls,
which would cover `x + n` at least under non-ANSI? Happy for that to be a
follow-up.
##########
spark/src/test/resources/sql-tests/expressions/array/sequence.sql:
##########
@@ -15,17 +15,175 @@
-- specific language governing permissions and limitations
-- under the License.
--- Routes sequence through the codegen dispatcher so behavior matches Spark
exactly.
+-- sequence(start, stop[, step]) for integral element types runs on the native
kernel
+-- (https://github.com/apache/datafusion-comet/issues/5349). Date and
timestamp sequences
+-- stay on the JVM codegen dispatcher and are exercised at the bottom of this
file.
statement
-CREATE TABLE test_sequence(a int, b int) USING parquet
+CREATE TABLE test_sequence(
+ b_start tinyint, b_stop tinyint, b_step tinyint,
+ s_start smallint, s_stop smallint, s_step smallint,
+ i_start int, i_stop int, i_step int,
+ l_start bigint, l_stop bigint, l_step bigint)
+USING parquet
+-- Row 2 descends, row 3 has start == stop, rows 4-6 carry NULLs in each
argument position.
statement
-INSERT INTO test_sequence VALUES (1, 5), (5, 1), (3, 3), (NULL, 5)
+INSERT INTO test_sequence VALUES
+ (1Y, 5Y, 1Y, 1S, 5S, 1S, 1, 10, 3, 1L, 5L, 2L),
+ (-3Y, -1Y, 1Y, 100S, 90S, -2S, 20, 2, -6, 9223372036854775802L,
9223372036854775807L, 1L),
+ (0Y, 0Y, 0Y, -5S, -5S, 0S, 7, 7, 0, -9223372036854775808L,
-9223372036854775800L, 3L),
+ (NULL, 5Y, 1Y, NULL, 5S, 1S, NULL, 10, 1, NULL, 5L, 1L),
+ (1Y, NULL, 1Y, 1S, NULL, 1S, 1, NULL, 1, 1L, NULL, 1L),
+ (1Y, 5Y, NULL, 1S, 5S, NULL, 1, 10, NULL, 1L, 5L, NULL)
+
+-- ============================================================================
+-- Explicit step, all four integral types
+-- ============================================================================
+
+query
+SELECT sequence(i_start, i_stop, i_step) FROM test_sequence
+
+query
+SELECT sequence(l_start, l_stop, l_step) FROM test_sequence
+
+-- Column step for the narrow integral types exercises the Byte/Short
monomorphizations
+-- of the native kernel, not just the literal-step shape.
+query
+SELECT sequence(b_start, b_stop, b_step) FROM test_sequence
+
+query
+SELECT sequence(s_start, s_stop, s_step) FROM test_sequence
+
+-- ============================================================================
+-- Default step: per-row start <= stop ? 1 : -1, both directions in one column
+-- ============================================================================
+
+query
+SELECT sequence(b_start, b_stop), sequence(s_start, s_stop) FROM test_sequence
+
+query
+SELECT sequence(i_start, i_stop), sequence(l_start, l_stop) FROM test_sequence
+
+-- ============================================================================
+-- Literal and mixed literal/column arguments
+-- ============================================================================
query
-SELECT a, b, sequence(a, b) FROM test_sequence
+SELECT sequence(1, 10), sequence(10, 1), sequence(5, 5), sequence(5, 5, 0)
--- literal arguments with step
query
SELECT sequence(1, 5), sequence(5, 1, -1), sequence(1, 10, 2)
+
+query
+SELECT sequence(1L, 9L, 2L), sequence(-128Y, -120Y), sequence(32760S, 32767S)
+
+-- On row 2 the source row is (i_start=20, i_stop=2, i_step=-6), so the
literal-step column
+-- asks for sequence(1, 2, 2) = [1] while the default-step column asks for
sequence(20, 25)
+-- = [20, 21, 22, 23, 24, 25]. The two columns disagreeing in direction on the
same row is
+-- intentional coverage, not an oversight.
+query
+SELECT sequence(1, i_stop, 2), sequence(i_start, 25) FROM test_sequence WHERE
i_start IS NOT NULL AND i_stop IS NOT NULL
+
+query
+SELECT sequence(CAST(NULL AS int), 5), sequence(1, CAST(NULL AS int)),
sequence(1, 5, CAST(NULL AS int))
+
+-- Integer.MIN_VALUE/MAX_VALUE bounds for int, and a sequence spanning zero
+query
+SELECT sequence(2147483642, 2147483647), sequence(-2147483648, -2147483643),
sequence(-3, 3, 3)
+
+-- ============================================================================
+-- sequence feeding explode, the common date-spine shape (with integers)
+-- ============================================================================
+
+query
+SELECT i_start, x FROM test_sequence LATERAL VIEW explode(sequence(i_start,
i_stop)) AS x WHERE i_start IS NOT NULL AND i_stop IS NOT NULL
+
+-- ============================================================================
+-- Error paths: step direction contradicts bounds, or zero step with start !=
stop
+-- ============================================================================
+
+query expect_error(Illegal sequence boundaries: 1 to 5 by -1)
+SELECT sequence(1, 5, -1)
+
+query expect_error(Illegal sequence boundaries: 10 to 2 by 3)
+SELECT sequence(10, 2, 3) FROM test_sequence LIMIT 1
+
+query expect_error(Illegal sequence boundaries: 1 to 5 by 0)
+SELECT sequence(1, 5, 0)
+
+-- ============================================================================
+-- Error paths: length exceeds MAX_ROUNDED_ARRAY_LENGTH
+-- ============================================================================
+
+query expect_error(the array size limit 2147483632)
Review Comment:
`SequenceBatchTooLarge` is the one behaviour in this PR that fails a query
Spark completes, and I cannot find a test for it anywhere. It is also close to
free to test, because the `total > i32::MAX` check runs in the first pass
before anything is allocated.
Could a case go in around here? Something like this trips it at the default
batch size and passes at half of it, so it also pins the remedy you documented:
```sql
statement
CREATE TABLE t_seq_ceiling(a INT, b INT) USING parquet
query expect_error(Lower `spark.comet.batchSize`)
SELECT sum(CAST(size(sequence(a, b)) AS BIGINT)) FROM t_seq_ceiling
```
with `a = 0, b = 262143` over 8192 rows in a single partition. I ran that
against this branch and it produces exactly the message you intended, and
lowering `spark.comet.batchSize` to 4096 makes it return Spark's answer
instead. That is the `sequence(0, 262143)` case from my first pass, now
confirmed end to end.
--
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]
