Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++][Dataset] Minimize Expression to a wrapper around compute::Function #26312

Closed
asfimport opened this issue Oct 16, 2020 · 1 comment
Closed

Comments

@asfimport
Copy link
Collaborator

asfimport commented Oct 16, 2020

The Expression class hierarchy was originally intended to provide generic, structured representations of compute functionality. On the former point they have been superseded by compute::

{Function, Kernel, ...}

which encapsulates validation and execution. In light of this Expression can be drastically simplified and improved by composition with these classes. Each responsibility which can be deferred implies less boilerplate when exposing a new compute function for use in datasets. Ideally any compute function will be immediately available to use in a filter or projection.

struct Expression {
  using Literal = std::shared_ptr<Scalar>;

  struct Call {
    std::shared_ptr<ScalarFunction> function;
    std::shared_ptr<FunctionOptions> options;
    std::vector<Expression> arguments;
  };

  util::variant<Literal, FieldRef, Call> value;
};

A simple discriminated union as above should be sufficient to represent arbitrary filters and projections: any expression which results in type bool is a valid filter, and any expression which is a Projection may be used to map one record batch to another.

Expression simplification (currently implemented in Expression::Assume) is an optimization used for example in predicate pushdown, and therefore need not exhaustively cover the full space of available compute functions.

Reporter: Ben Kietzman / @bkietz
Assignee: Ben Kietzman / @bkietz

Related issues:

PRs and other links:

Note: This issue was originally created as ARROW-10322. Please see the migration documentation for further details.

@asfimport
Copy link
Collaborator Author

Neal Richardson / @nealrichardson:
Issue resolved by pull request 8894
#8894

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants