bug: log2 produces different values than Spark in some cases

[andygrove]

Describe the bug

I discovered this bug using Comet Fuzz.

SELECT Log2(c4) FROM test0

[ERROR] Spark and Comet produced different results.

Spark Plan

*(1) Project [LOG2(cast(c4#4 as double)) AS LOG2(c4)#13927]
+- *(1) ColumnarToRow
   +- FileScan parquet [c4#4] Batched: true, DataFilters: [], Format: Parquet, Location: InMemoryFileIndex(1 paths)[file:/Users/andy/git/apple/comet-fuzz/test0.parquet], PartitionFilters: [], PushedFilters: [], ReadSchema: struct<c4:tinyint>

Comet Plan

*(1) ColumnarToRow
+- CometProject [LOG2(c4)#13931], [LOG2(cast(c4#4 as double)) AS LOG2(c4)#13931]
   +- CometScan parquet [c4#4] Batched: true, DataFilters: [], Format: CometParquet, Location: InMemoryFileIndex(1 paths)[file:/Users/andy/git/apple/comet-fuzz/test0.parquet], PartitionFilters: [], PushedFilters: [], ReadSchema: struct<c4:tinyint>

Results

Spark produced 200 rows and Comet produced 200 rows.
First difference at row 0:
Spark: [1.584962500721156]
Comet: [-Infinity]

Steps to reproduce

No response

Expected behavior

No response

Additional context

No response

[parthchandra]

For anyone debugging this -
println!("{}", 3f64.log2()); produces 1.584962500721156 which matches the expected value

[prashantksharma]

@andygrove

I would like to work on this. Can I take it up ?

[PedroMDuarte]

Is it possible that the difference report is not sorting the data in the same way for spark and comet? I'm surprised by the discrepancy:

Spark: [1.584962500721156]
Comet: [-Infinity]

the 1.58 value is log2(3) whereas -Infinity is log2(0).

Running without and with comet enabled I do see a discrepancy for log2(0) as follows:

spark
+---+------------------+-----------------+
|  a|           LOG2(a)|(LOG2(a) IS NULL)|
+---+------------------+-----------------+
|  0|              null|             true|
|  1|               0.0|            false|
|  2|               1.0|            false|
|  3| 1.584962500721156|            false|
|  4|               2.0|            false|
|  5| 2.321928094887362|            false|
|  6| 2.584962500721156|            false|
|  7| 2.807354922057604|            false|
|  8|               3.0|            false|
|  9|3.1699250014423126|            false|
+---+------------------+-----------------+

spark with comet
+---+-----------------+-----------------+
|  a|          LOG2(a)|(LOG2(a) IS NULL)|
+---+-----------------+-----------------+
|  0|        -Infinity|            false|
|  1|              0.0|            false|
|  2|              1.0|            false|
|  3|1.584962500721156|            false|
|  4|              2.0|            false|
|  5|2.321928094887362|            false|
|  6|2.584962500721156|            false|
|  7|2.807354922057604|            false|
|  8|              3.0|            false|
|  9|3.169925001442312|            false|
+---+-----------------+-----------------+

[andygrove]

Is it possible that the difference report is not sorting the data in the same way for spark and comet?

The queries all have an ORDER BY clause and single partition, so should be deterministic. There is always the possibility that there is a bug in the fuzz testing tool though.

[andygrove]

The queries all have an ORDER BY clause and single partition, so should be deterministic. There is always the possibility that there is a bug in the fuzz testing tool though.

Actually, @PedroMDuarte is correct. This issue was generated from an earlier version of the tool before the ordering logic was improved. The root cause is likely the log2(0) behavior being different between Comet and Spark.

[PedroMDuarte]

Thanks for replying Andy. I was looking at this as a good first issue for me. I browsed through the code but couldn't determine where the issue should be addressed. Any pointers would be much appreciated.

…

On Sun, Jun 2, 2024, 5:58 PM Andy Grove ***@***.***> wrote: The queries all have an ORDER BY clause and single partition, so should be deterministic. There is always the possibility that there is a bug in the fuzz testing tool though. Actually, @PedroMDuarte <https://github.com/PedroMDuarte> is correct. This issue was generated from an earlier version of the tool before the ordering logic was improved. The root cause is likely the log2(0) behavior being different between Comet and Spark. — Reply to this email directly, view it on GitHub <#485 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHA5KXZOUVOIY4GUROXYKDZFOIQ3AVCNFSM6AAAAABIND3OG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBUGAZTIMRTGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[andygrove]

It looks like log calls get mapped to this protobuf type:

message ScalarFunc {
  string func = 1;
  repeated Expr args = 2;
  DataType return_type = 3;
}

In planner.rs we have create_scalar_function_expr, which calls create_comet_physical_fun, which has special handling for scalar functions that we have implemented in comet but then falls back to DataFusion built-in functions for anything else:

            if let Ok(fun) = BuiltinScalarFunction::from_str(fun_name) {
                Ok(ScalarFunctionDefinition::BuiltIn(fun))

So this is where we get the DataFusion log functions.

Log(0) is undefined behavior and different systems return different results.

I wonder if we should just document this behavior rather than try and make Comet compatible.

If we do want to match Spark, we could wrap the log expr in another expression, such as if(expr == 0, null, log(expr)). Comet does already have support for if expressions:

IfExpr::new(if_expr, true_expr, false_expr)

[eejbyfeldt]

If we do want to match Spark, we could wrap the log expr in another expression, such as if(expr == 0, null, log(expr)). Comet does already have support for if expressions:

IfExpr::new(if_expr, true_expr, false_expr)

The predicate should probably be expr <= 0 instead of expr == 0 to match Spark for all cases since that is how it implemented in Spark: https://github.com/apache/spark/blob/v3.5.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala#L92-L95