Properly process Final Selection in Reduce().

[spershin]

Summary:
In Reduce() we ignored the fact that Final Selection might have been false before we set it to false ourselves.
In that case we need to use different SelectivityVector.

Differential Revision: D39121620

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	657734d
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6317ba7a0872310008ccd416

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D39121620

[mbasmanova]

@spershin Thank you for the fix. Some comments and questions below.

[mbasmanova]

This code can be simplified if it is moved after the finalSelection and isFinalSelection.

const auto& finalSelectionRows = *context->finalSelection();

[mbasmanova]

nit: use TypedExprPtr

[mbasmanova]

Thank you for refactoring.

[mbasmanova]

Does this test fail before the fix?

[spershin]

Good question. Unfortunately - not.
I only made sure that we hit the new code path.

[mbasmanova]

To reproduce the problem we need to

1. make it so that context->finalSelection() is larger than 'rows'.
2. use outputFunction that relies of EvalCtx::moveOrCopyResult

To satisfy (1), we can wrap reduce call in an IF statement. To satisfy (2), we can use row_constructor.

Here is an example of a test that fails without a fix and succeeds with the fix.

TEST_F(ReduceTest, finalSelection) {
  vector_size_t size = 1'000;
  auto inputArray = makeArrayVector<int64_t>(
      size,
      modN(5),
      [](auto row, auto index) { return row + index; },
      nullEvery(11));
  auto input = makeRowVector({
      inputArray,
      makeFlatVector<int64_t>(
          size, [](auto row) { return row; }, nullEvery(11)),
  });
  registerLambda(
      "sum_input",
      rowType("s", BIGINT(), "x", BIGINT()),
      input->type(),
      "s + x");
  registerLambda(
      "row_output",
      rowType("s", BIGINT()),
      input->type(),
      "row_constructor(s)");

  auto result = evaluate<RowVector>(
      "if (c1 < 100, row_constructor(c1), reduce(c0, 10, function('sum_input'), function('row_output')))",
      input);

  auto expectedResult = makeRowVector({makeFlatVector<int64_t>(
      size,
      [](auto row) -> int64_t {
        if (row < 100) {
          return row;
        } else {
          int64_t sum = 10;
          for (auto i = 0; i < row % 5; i++) {
            sum += row + i;
          }
          return sum;
        }
      },
      nullEvery(11))});
  assertEqualVectors(expectedResult, result);
}

The failure before the fix is:

/Users/mbasmanova/cpp/velox-1/velox/vector/tests/VectorTestBase.cpp:98: Failure
Value of: expected->equalValueAt(actual.get(), i, i)
  Actual: false
Expected: true
at 1: expected {1}, but got {-6799976246779207263}

Notice that the fix is needed only for the "Apply output function" code. The code that applies input function doesn't need the fix.

Final selection is used for two purposes: (1) ensure lazy vectors are loaded for all the necessary rows; (2) ensure that pre-computed results are preserved. (1) doesn't apply when evaluating lambda functions since all inputs have already been loaded. (2) doesn't apply to "apply input function" here because the partialResult is created from scratch in this function and therefore doesn't have any useful data outside of 'rows'.

Hope this helps.

CC: @bikramSingh91

[spershin]

Thanks, @mbasmanova, this is very helpful.
No way any time soon I would try to use row_constructor() or anything like that to reproduce the issue.
I was thinking about if(), but that alone wouldn't reproduce.

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D39121620

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D39121620