ApproxPercentile fails with xxx away from acceptable range

kgpai commented 7 months ago

Description

When running with Nan/Inf support in fuzzer approx_percentile seems to fail with following error . PR : https://github.com/facebookincubator/velox/pull/8732 .

Error Reproduction

Try seed : 46435304 in Aggregation fuzzer against the linked PR above.

Relevant logs

I20240212 15:13:07.450784 19839653 AggregationFuzzerBase.cpp:364] Executing query plan: 
-- Aggregation[SINGLE a0 := approx_percentile(ROW["c0"],ROW["c1"]) mask: m0] -> a0:ARRAY<DOUBLE>
  -- Values[1000 rows in 10 vectors] -> c0:DOUBLE, c1:ARRAY<DOUBLE>, m0:BOOLEAN
I20240212 15:13:07.453105 19844862 Task.cpp:1085] All drivers (1) finished for task test_cursor 608 after running for 2 ms.
I20240212 15:13:07.453125 19844862 Task.cpp:1769] Terminating task test_cursor 608 with state Finished after running for 2 ms.
I20240212 15:13:07.460280 19844863 Task.cpp:1085] All drivers (3) finished for task test_cursor 609 after running for 3 ms.
I20240212 15:13:07.460305 19844863 Task.cpp:1769] Terminating task test_cursor 609 with state Finished after running for 3 ms.
E20240212 15:13:07.460641 19839653 ApproxPercentileResultVerifier.h:154] approx_percentile(pct: 0.95, accuracy: 0.0133) is more than 0.0133 away from acceptable range of [0.683386, 0.811912]. Difference: 0.138088
E20240212 15:13:07.460682 19839653 Exceptions.h:69] Line: /Users/kpai/src/velox/velox/exec/fuzzer/AggregationFuzzer.cpp:1084, Function:compareEquivalentPlanResults, Expression: verifier->verify(resultOrError.result) Aggregation results failed custom verification, Source: RUNTIME, ErrorCode: INVALID_STATE
I20240212 15:13:07.486898 19839653 AggregationFuzzerBase.cpp:655] Persisted aggregation plans to /tmp/aggregate_fuzzer_repro/velox_aggregationVerifier_nDkFYV/plan_nodes
libc++abi: terminating due to uncaught exception of type facebook::velox::VeloxRuntimeError: Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: Aggregation results failed custom verification
Retriable: False
Expression: verifier->verify(resultOrError.result)
Function: compareEquivalentPlanResults
File: /Users/kpai/src/velox/velox/exec/fuzzer/AggregationFuzzer.cpp
Line: 1084

Yuhta commented 7 months ago

Shall we just throw exception if input contains NaN? There is no well defined order for NaN so there is no way to get a percentile that makes sense if input contains NaN.

Another way is implicitly treat NaN same as NULL. But I feel it should be left to user to do the conversion explicitly.

A third way is returning NaN in this case. We can check if Presto Java is taking this path.

kgpai commented 7 months ago

@Yuhta I will validate against Presto behavior and let you know

Yuhta commented 2 months ago

I think this is another NaN behavior problem, CC: @bikramSingh91

facebookincubator / velox