ARROW-7391: [C++][Dataset] Remove Expression subclasses from bindings

[bkietz]

Serialization is implemented by converting Expressions to Arrays then writing a tiny IPC file. This is a ridiculous way to serialize Expressions but it should be acceptable since these classes are destined to be replaced by arrow::engine::Expr

[github-actions]

https://issues.apache.org/jira/browse/ARROW-7391

[nealrichardson]

A couple of questions on the R bindings. Also, IIUC you can delete https://github.com/apache/arrow/blob/master/r/R/expression.R#L122-L163 and refactor that file a bit to match how you're not really specifying the Expression subclasses anymore.

[jorisvandenbossche]

Personally, I am not sure if I find the Expression.field, Expression.and_, etc API necessarily better than the current FieldExpression, AndExpression, etc.
Both equally tie us to a certain python API: whether we use static methods or subclass constructors to construct the Expression objects doesn't really matter in the end IMO (they still expose an Expression class with methods, and a constructor like AndExpression could also simply be a wrapper to construct a base expression object as is done now in Expression.and_).

To be clear, I am fully on board with the the general clean-up and reducing the C++ API used from the python side. So I am only talking about the Python (user-facing) API. But for that, I am wondering: do we actually need to expose those constructors publicly? I think that the current ds.field(..) and ds.scalar(..) functions should be sufficient, as all other expressions can be created based on those using methods or binary operations?

[kszucs]

Agree that exposing ds.field and ds.scalar should be sufficient on the python side.

[bkietz]

@kszucs @jorisvandenbossche PTAL

[nealrichardson]

@github-actions autotune everything

[jorisvandenbossche]

Thanks for the updates!

[fsaintjacques]

Why?

[bkietz]

This allows Iterator::RangeIterator to be copied, which is a requirement on iterators https://en.cppreference.com/w/cpp/named_req/Iterator

We haven't caught this before because we only use Iterator::begin and Iterator::end in the context of range-for loops which don't require it. However cython wraps the range-for loop does, however, which is why I noticed the concept error.

[fsaintjacques]

What is this doing?

[bkietz]

This is ensuring that /\w+Type::type_id/ is a defined symbol in libarrow.so. Previously these have only been used within a translation unit which links into libarrow.so, but now I'm using them in filter.cc which gets linked to libarrow_dataset.so. If you remove these you'll find the linker emits an error because they're undefined in libarrow.so

[wesm]

This seems like a quirk due to the use of ARROW_EXPORT, would I be correct that this issue would not occur without the export/import business?

[fsaintjacques]

Use GetView since you copy it right after in a Buffer, that'll remove one memcpy.

[bkietz]

Buffer::FromString wraps the allocation of the std::string without copying

[nealrichardson]

@bkietz please update your version of the R roxygen2 package and re-run make doc (the autotune bot is failing because of python 2.7 or something)

[wesm]

+1, thanks for doing this -- I think this will save us some headache down the road

[wesm]

Seems likely we'll want to define a (Flatbuffers?) serialization protocol protocol for expressions in the not-too-distant future, though this certainly works for now! =)

[wesm]

This seems like a quirk due to the use of ARROW_EXPORT, would I be correct that this issue would not occur without the export/import business?