fix: Avoid throw exception in toSubfieldFilter

[MBkkt]

Why?

Because in Axiom now code written like this

    try {
      auto pair = velox::exec::toSubfieldFilter(typedExpr, evaluator);
      if (!pair.second) {
        remainingConjuncts.push_back(std::move(typedExpr));
        continue;
      }
      pushdownConjuncts.push_back(typedExpr);
    } catch (const std::exception&) {
      remainingConjuncts.push_back(std::move(typedExpr));
    }
  }

it's not only ineffective, it's inconvenient to debug (you cannot use break on throw, because this function will throw even when everything fine)

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	a6dba3a
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/6908fcf1bf97580008272663

[Yuhta]

Can we add a bool flag or another function (e.g. toSubfieldFilterNoThrow) to introduce this behavior? Silently suppressing errors should not be the default behavior.

[MBkkt]

@Yuhta This function already returns null in some cases (with implicit error) and in Axiom this error anyway ignored.

Are any other usages where this information is used?

[Yuhta]

@MBkkt Ideally null should be returned only for unsupported expression (valid expression that cannot be converted to single field filters). For invalid expression, we should fail early as soon as we can (unless there is some reason preventing us from doing so).

[MBkkt]

@Yuhta ok, sounds reasonable to me, I will change this

[MBkkt]

@Yuhta I think for this PR only for unsupported expressions null returned

[Yuhta]

I see, this method is a little weird and very limited. Ideally people usually do is to extract a list of filters from a conjunctive expression. It seems this method was created only for testing purpose so the behavior is a little weird. We may need to revisit this method and exam where it is used later.

[mbasmanova]

I wonder if we should remove the logic for handling "or"... I don't think Axiom expects that logic.

[Yuhta]

Discussed with @mbasmanova and we probably should document this method first to make the behavior and intended usage clear before changing it.

[mbasmanova]

This API is not documented. Let's document it before / at the same time as making changes to it. Also, see #15384

[mbasmanova]

Let's make sure to add tests for this logic.

[mbasmanova]

It might be clearer to change return type to std::optional and return std::nullptr.

[MBkkt]

I think it's worse.
What is semantics of nullptr filter in such case?