Remove INCOMPAT for NormalizeNanAndZero, KnownFloatingPointNormalized

[mythrocks]

Removed INCOMPAT flag for FP Normalization functions.

[kuhushukla]

Is it worth considering removing the has nan check in hash aggregate at this time?

[revans2]

build

[revans2]

I wish we had a way to flag if a test was marked as incompat, but didn't need it. The pyspark has aggregate tests also have incompat on several of them for this reason.

[mythrocks]

I wish we had a way to flag if a test was marked as incompat...

I should try and remove @incompat for the obvious ones. I'll update here.

[mythrocks]

I have removed the @incompat for the NaN/Zero test cases added in #160.

I tried going through and removing the @allowed_on_cpu for other tests in hash_aggregate_test.py, but those are more involved. (The NOT_ON_GPU results from other reasons.)

[revans2]

buil

[revans2]

build

[mythrocks]

Is it worth considering removing the has nan check in hash aggregate at this time?

@kuhushukla, I'm looking into this now.

[mythrocks]

@kuhushukla, I have now removed the NaNs check from GpuHashAggregateMeta. HashAggregateSuite and hash_aggregate_test.py pass.

That's the only change in the last commit (0a241a8). Could you please confirm that I haven't missed anything?

[mythrocks]

build

[kuhushukla]

@kuhushukla, I have now removed the NaNs check from GpuHashAggregateMeta. HashAggregateSuite and hash_aggregate_test.py pass.

That's the only change in the last commit (0a241a8). Could you please confirm that I haven't missed anything?

I dont think any of the tests from before what u have just added test for the grouping key as Nan. I suspect we will hit some issues around that but since all of the legacy tests were written with has_nans=false in mind I would probably not rely on them too much and possibly look at adding more tests for it. This change then makes me think if it should be a separate PR at this point. We want coherency when we say this data has nans and now we can normalize them but we have hit issues with sorting nans and such before so maybe testing and identifying what does or does not work with it on different operators and turning it off at aggregate operator level would be better. I apologize if I have mislead you as far as this PR goes but we need to look at how to use has_nans properly here or as a follow on.

[mythrocks]

The hasNans flag is fraught with peril. :/ This flag conflates a couple of scenarios:

1. NaN in GBY expressions
2. NaN in the aggregation column

I'd be delighted to revert my change to GpuHashAggregateMeta. But based on my testing, I think it might actually be safe for GpuHashAggregate to bank on normalization to do the GBY correctly. The first point above should be covered.

I have added more aggregation functions to the NaN/Zero normalization tests. These tests succeed with and without the hasNans check in GpuHashAggregateMeta.
Incidentally, I've uncovered an unrelated issue (#194) with COUNT(DISTINCT float_column) that I've added an xfail test for. This fails regardless or the GpuHashAggregateMeta hasNans check.

[mythrocks]

build

[revans2]

build