Smallfu666 opened a new issue, #5389:
URL: https://github.com/apache/datafusion-comet/issues/5389
### Describe the bug
`DataGenOptions.allowNull` defaults to `true`, but a `Boolean`, `Byte`,
`Short` or `Integer` column
declared `nullable = true` never receives a single null.
Those four derive their values from the `LongType` generator and then unbox
each element
(`spark/src/main/scala/org/apache/comet/testing/FuzzDataGenerator.scala:200-212`):
```scala
case DataTypes.BooleanType =>
generateColumn(r, DataTypes.LongType, numRows, options)
.map(_.asInstanceOf[Long].toShort)
.map(s => s % 2 == 0)
case DataTypes.ByteType =>
generateColumn(r, DataTypes.LongType, numRows, options)
.map(_.asInstanceOf[Long].toByte)
case DataTypes.ShortType =>
generateColumn(r, DataTypes.LongType, numRows, options)
.map(_.asInstanceOf[Long].toShort)
case DataTypes.IntegerType =>
generateColumn(r, DataTypes.LongType, numRows, options)
.map(_.asInstanceOf[Long].toInt)
```
The `LongType` generator does emit nulls, one in fifty:
```scala
case DataTypes.LongType =>
Range(0, numRows).map(_ => {
r.nextInt(50) match {
case 0 if options.allowNull => null
...
```
but the elements are typed `Any`, so `_.asInstanceOf[Long]` compiles to
`scala.runtime.BoxesRunTime.unboxToLong`, which returns `0` for a null
reference rather than
throwing. Every generated null becomes an ordinary value:
| type | a generated `null` becomes |
| --- | --- |
| `ByteType` | `0` |
| `ShortType` | `0` |
| `IntegerType` | `0` |
| `BooleanType` | `true`, since `0 % 2 == 0` |
`Long`, `Float`, `Double`, `String` and `Array` keep their nulls, and
`Binary` keeps them too
because it derives from `String` through a `case _ => null` arm rather than
an unboxing cast. So the
gap is silent and type-dependent: a suite fuzzing a nullable `Long` column
really is testing nulls,
and the identical-looking `Int` column beside it is not.
### This has already let a native bug through an end-to-end test
Not a theoretical coverage concern. PR #2630 (`00922cfc4`, "Fallback to
Spark for lpad/rpad for
unsupported arguments & fix negative length handling") introduced, **in the
same commit**:
- `let length = length.unwrap();` in `spark_read_side_padding_internal`,
which aborts the native
process on a null length, and
- `CometStringExpressionSuite.testStringPadding`, whose schema declares
`StructField("len", DataTypes.IntegerType, nullable = true)`, fed by
`FuzzDataGenerator` over 1000
rows and run through `checkSparkAnswerAndOperator`, so natively.
On paper that test covers exactly the case the `unwrap` cannot handle. It
has passed ever since,
because the column contains no nulls. I re-ran it on current `main` to
confirm: 33 tests, 33
succeeded.
### Verification
Unboxing behaviour, executed against `scala-library-2.12.20`:
```java
long v = scala.runtime.BoxesRunTime.unboxToLong(null); // 0
int i = (int) v; // 0
```
`generateDataFrame` does not reinstate nulls afterwards; it only transposes
the generated columns
into rows (`FuzzDataGenerator.scala:156-172`).
### Expected behavior
A `Boolean`, `Byte`, `Short` or `Integer` column declared `nullable = true`
with `allowNull` enabled
contains nulls, so nullable fuzz coverage means what it says.
### Additional context
A fix should come with an assertion that nulls were actually produced for
each nullable type, so the
property cannot regress silently again. `DataGeneratorSuite` is the natural
home for it.
Worth restoring the coverage a few types at a time rather than in one
change: the generator is
shared by expression, math, string, aggregate, Parquet and Iceberg suites,
so real nulls are likely
to surface latent failures that are easier to triage in small batches.
The same audit found that `Decimal`, `Date`, `Timestamp` and `TimestampNTZ`
never consult
`allowNull` at all, so they are never null either, by omission rather than
by unboxing. Those types
are outside this issue's Boolean/Byte/Short/Integer scope and can be tracked
separately.
--
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]
