Update struct_minmax_util to experimental row comparator

[divyegala]

Description

Par of #11844

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[ttnghia]

I've already worked on it: #10811.

It was stalled because of performance regression. Still need to wait for more performance optimization before continuing.

[divyegala]

Still need to wait for more performance optimization before continuing.

@ttnghia

@GregoryKimball and I discussed this and the performance regressions don't look significant to us. If you prefer, can you bring your PR up-to-date and we can run a set of microbenchmarks on it to determine if the minor cost is just a single run and amortized over multiple runs?

[ttnghia]

I would prefer to do it after all the optimization mentioned above came in. By doing so we can have the performance regression (if any) being minimized.

[vyasr]

I asked about moving forward on Nghia's PR right around when this one got made it looks like #10811 (comment). Can we discuss a concrete plan for moving forward here?

• What benchmarks do we need to see? The other PR already has some reported.
• What performance numbers do we need to hit?
• Have we done any investigation at all to understand the causes of the regression?

[ttnghia]

I think this is the reason that fails many tests in this PR. In the meantime I don't have a clear way to fix it. Need to think further.

[divyegala]

Only this test is failing in this PR now. Any ideas why?

StructReductionTest.StructReductionMinMaxWithNulls

[ttnghia]

I'm not sure. Can you print out the output of that test to see what it is?

[divyegala]

[ RUN      ] StructReductionTest.StructReductionMinMaxWithNulls
/opt/conda/conda-bld/work/cpp/tests/utilities/column_utilities.cu:289: Failure
Expected equality of these values:
  lhs_null_count
    Which is: 1
  rhs_null_count
    Which is: 0
Google Test trace:
/opt/conda/conda-bld/work/cpp/tests/reductions/reduction_tests.cpp:2565:  <--  line of failure

[ttnghia]

That is the log, not the output. Add this to line 2565:

cudf::test::print(struct_result->view());

[divyegala]

It looks like this was already a pre-conditon for min where top-level NULLs were pushed to AFTER

[ttnghia]

Yes. So the correct output is {null, null} which contains nulls only in the children, not top level.
In other word: The failed test has nulls excluded (nulls are larger than non-nulls) in the children level.

[divyegala]

Ah I see, I don't understand why this is happening. Fault with code I wrote or lex comparator?

[ttnghia]

The previous approach flattens the input structs column into a table. The null order here will have size equal to number of table columns:

  if (is_min_op) {
      null_orders = flattened_input->null_orders();

So we could just change the first null order element and the top level null order will be different from all children level null order.

In the new approach, you always have just one input column and always have the null order array having one element. The row comparator will then use that unique null order for all top level and children level.

I can think of only one solution to overcome this: continue flattening the input and modify the null order vector, then pass everything into the row comparator.

[ttnghia]

And that is only needed for min. For max, all null order values are the same so we don't modify the null order array.

[ttnghia]

@vyasr My potential optimization mentioned earlier has failed thus I don't block this anymore.

• What benchmarks do we need to see? The other PR already has some reported.

Since this is touching struct min/max, we should run the benchmark on struct min/max on groupby scan and reduction. I ran such benchmark here so this PR just need to reran them: #10811.

• What performance numbers do we need to hit?

We should not have significant performance regression with the benchmark above.

• Have we done any investigation at all to understand the causes of the regression?

The cause of regression before was probably due to register usage as I investigated: #10811 (comment).

[vyasr]

@ttnghia can we close #10811 and use this PR instead, or would you prefer the reverse? Is there any difference, or code that needs to be ported from one to the other?

@divyegala let's repoint this PR at 23.08, don't think there's anything urgent to get in for this release here.

[ttnghia]

@vyasr sure this should close #10811.

[vyasr]

@divyegala I've retargeted and merged latest 23.08. There are a few comments to address, then I think this PR should be close.

[divyegala]

@vyasr @ttnghia this PR is up to date now. There are failing tests for scans/reductions when NULLs are present, although I'm not really sure how to diagnose why they are occurring. Any pointers?

[vyasr]

Perhaps I'm missing something: I see that you removed the old tests verifying that this code path failed loudly, but shouldn't we add new tests verifying the now supported behavior?

[vyasr]

Approving after some offline discussions. The main new feature added by this PR is nesting lists with structs (whereas previous only pure struct nesting was supported), but in other such PRs we have not added testing of the list case either. We're relying on existing tests of the comparator to validate those specific different cases.

[divyegala]

/merge