ARROW-7878: [C++][Compute] Draft LogicalPlan classes #6506

fsaintjacques · 2020-02-28T21:45:34Z

Draft of the LogicalPlan basic classes for the query engine. One should read the classes in this order for review:

ExprType/Expr
LogicalPlanBuilder/LogicalPlan
Catalog

github-actions · 2020-02-28T21:49:52Z

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

fsaintjacques · 2020-03-01T13:03:43Z

cpp/src/arrow/engine/expression.h

+ private:
+  explicit FieldRefExpr(std::shared_ptr<Field> field);
+
+  std::shared_ptr<Field> field_;


This will be replaced with the class in ARROW-7421.

andygrove · 2020-03-01T18:03:09Z

cpp/src/arrow/engine/logical_plan_test.cc

+      field("bool", boolean()),
+      field("i32", int32()),
+      field("u64", uint64()),
+      field("f32", uint32()),


This looks like a mismatch for the f32 type?

andygrove · 2020-03-01T18:05:20Z

This is exciting! This may be the final motivation I need to start coding in C++ again.

cpp/src/arrow/engine/catalog.h

pitrou

Seems like the LogicalPlanBuilder isn't finished. Some random comments below.

cpp/src/arrow/engine/expression.h

pitrou · 2020-03-02T16:27:19Z

cpp/src/arrow/engine/logical_plan.h

+  std::shared_ptr<Catalog> catalog;
+};
+
+class LogicalPlanBuilder {


Hmm... I suppose LogicalPlanBuilder will grow a Build method to build a LogicalPlan? How will it work? The builder doesn't seem to keep track of any state.

pitrou · 2020-03-02T16:30:07Z

cpp/src/arrow/testing/gmock.h

@@ -0,0 +1,31 @@
+// Licensed to the Apache Software Foundation (ASF) under one


Call it "gmock_util.h"?

cpp/src/arrow/testing/gmock.h

cpp/src/arrow/engine/catalog.h

bkietz

This is looking great, thanks @fsaintjacques !

cpp/src/arrow/engine/catalog.h

cpp/src/arrow/engine/expression.h

bkietz · 2020-03-03T20:07:19Z

cpp/src/arrow/engine/expression_test.cc

+
+  ASSERT_OK_AND_ASSIGN(auto empty, EmptyRelExpr::Make(schema_1));
+  EXPECT_THAT(empty->type(), ExprType::Table(schema_1));
+  EXPECT_THAT(empty->schema(), PtrEquals(schema_1));


For one thing, the console shows expression_test.cc:205 instead of gtest_util.cc:206.

bkietz · 2020-03-03T20:10:28Z

cpp/src/arrow/engine/logical_plan.h

+  ResultExpr Filter(const std::shared_ptr<Expr>& input,
+                    const std::shared_ptr<Expr>& predicate);
+
+  /// \brief Project (mutate) columns with given expressions.


I thought the same when I read this. Could we leave the dplyr specific alias (Mutate method, comment is fine) to R?

bkietz · 2020-03-03T20:51:46Z

cpp/src/arrow/engine/expression.h

+  static Result<std::shared_ptr<ProjectionRelExpr>> Make(
+      std::shared_ptr<Expr> input, std::vector<std::shared_ptr<Expr>> expressions);
+
+  const std::vector<std::shared_ptr<Expr>> expressions() const { return expressions_; }


Preface: I am assuming that ProjectionRelExpr will always have shape TABLE, is that incorrect?

I would have expected ProjectionRelExpr to be parametrized not by a vector<Expr> but by ordered_map<string, Expr> or ordered_map<FieldRefExpr, Expr>. (Maybe you plan to have the key value pairs be implicit in an equality expression?)

For example

project(some_table, { a: field("alpha"), // result's column "a" will be column "alpha" from some_table b: scalar(3), // result's column "b" will be the scalar 3 broadcast with "a" })

No, I'll have an AliasExpr(expr, name) for this. I also intend to have each expression have the equivalent of python's repr which is a "short" print, I'll use this short print as the inferred field name. The AliasExpr overrides this and pass down the input expr.

sqlite> SELECT * FROM tmp; a|b 1|2 sqlite> SELECT a+1, b FROM tmp; a+1|b 2|2 sqlite> SELECT a+1, a+b FROM tmp; a+1|a+b 2|3 sqlite> SELECT AVG(a) + 1, a+b FROM tmp; AVG(a) + 1|a+b 2.0|3 sqlite> SELECT AVG(a) + 1 as mean_plus_one, a+b FROM tmp; mean_plus_one|a+b 2.0|3

And to answer your question, all RelExpr will have Table shape (expect maybe Aggregate which can reduce to a single scalar), that's their common point.

Okay, please add AliasExpr (or a TODO in the doccomment here). Out of curiosity, what will the field name for a projected Expr be if it is neither an AliasExpr nor a FieldRefExpr?

cpp/src/arrow/engine/logical_plan_test.cc

cpp/src/arrow/engine/type_fwd.h

kszucs · 2020-03-05T16:06:58Z

Perhaps I'm just too used to the abstractions in ibis, but I'd separate the operations from the expressions like the basic implementation in the compute namespace.

I'm afraid that if we need to encode the same functionality using inheritance will end up a less maintainable codebase.

bkietz · 2020-03-05T19:00:17Z

@kszucs could you give a concrete example of the flexibility we'd gain by separating those?

fsaintjacques · 2020-03-11T14:47:32Z

cpp/src/arrow/engine/expression.h

+    // The expression yields a Scalar, e.g. "1".
+    SCALAR,
+    // The expression yields an Array, e.g. "[1, 2, 3]".
+    ARRAY,


One unknown here is I don't know if we need to differentiate from Array and Column. For example, a user can still pass an "inline" array. Some operators (e.g. FilterRelExpr) requires that the predicate/mask has the same number of elements than the input table.

Is there an operator which doesn't require this? For example, I think In's argument will be ExprType::Scalar|Array(list(some_type())) rather than a top level array

What the difference between Array and Column, the latter one is named, right?
I don't think we need to differentiate because their shape is identical.

@kszucs no the main difference would be an implicit length constraint; an expression of Column shape would have equal length to all sibling expressions of Column shape.

@fsaintjacques what cases do you have in mind where an Array would have differing length from a Column?

I think the main difference is knowledge of the length of the array. Column has an implicit that the length is the same as the referenced table. Where a table may or not have this.

An example, a user could have computed the mask out-of-band in panda because we're missing a functionality. In this case, from the perspective of the LogicalPlan, this is a value array that does not come from an expression of a column. It needs to validate the length. The same goes for any operators, e.g. in projection I could provide an array literal to concatenate to an existing table as a new column.

The case you raise is interesting, but sounds more like something we would support through a custom Expr subclass than through a (guessing) first class ArrayExpr.

WRT the implicit length assumption: we'll still need to insert validation for this on translation to a physical plan, since a non-custom expression could still result in Columns of differing length:

# example 1: # no custom, but one column is filtered while the other isn't # so they probably won't have the same length And( Filter(FieldRef("int32_column"), FieldRef("mask")), FieldRef("bool_column")) # example 2: # custom; no guarantee that my_pandas_mask has equal size to int32_column # physical plan must include a length check Filter(FieldRef("int32_column"), CustomArrayExpr(my_pandas_mask))

The physical plan builder should recognize when the column length assumption is not guaranteed by an expression tree and insert length validation nodes. (Example 1 is a toy; rather than inserting a length validation node we should probably raise an error on translation to a physical plan since for non trivial mask it must break the column length assumption.)

kszucs · 2020-03-11T15:28:26Z

Could you give me an example expression for a query like

SELECT * FROM table WHERE table.a >= table.b

fsaintjacques · 2020-03-11T15:43:08Z

Could you give me an example expression for a query like
SELECT * FROM table WHERE table.a >= table.b 

t = Scan(table)
a = FieldRefExpr(t, "a")
b = FieldRefExpr(t, "b")
return FilterRelExpr(t, GreaterEqualThanExpr(a, b))

kszucs · 2020-03-11T17:08:09Z

Ok, so you encode the operation with inheritance in the expression. This means that all expression type needs to have a corresponding type enum and the visitors will have the same amount of overloads? (as comparison in ibis we have ~250 operations and ~60 expression types)

Sorry, I accidentally edited your comment.

wesm · 2020-03-13T14:14:08Z

Sorry I'm late to the party do I still have time to review this? I'll spend time on it today

fsaintjacques · 2020-03-13T14:25:47Z

@wesm I'm still fixing portability issues and adding more operators, e.g. AggregateFn and AggregateRel. It's not too late and we should review thoroughly this one, especially someone with more data frame view than my database centric view.

wesm

I left some comments (some cosmetic, some design questions) on some of the basic things that jumped out at me. I think we will want to invest some time in prototyping desired APIs based on more complex SQL queries. The document https://docs.ibis-project.org/sql.html could be useful to provide a list of different types of SQL concepts to try to capture.

I'd also like to make sure we're thinking about column expressions on nested types, consider for example the APIs in Ibis (as a strawman) for dealing with nested types in e.g. postgres

https://github.com/ibis-project/ibis/blob/master/ibis/sql/postgres/tests/test_functions.py#L1062

thanks again for getting this started!!

wesm · 2020-03-13T19:15:22Z

cpp/cmake_modules/DefineOptions.cmake

@@ -173,6 +173,8 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")

  define_option(ARROW_DATASET "Build the Arrow Dataset Modules" OFF)

+  define_option(ARROW_ENGINE "Build the Arrow Query Engine Modules" OFF)


Don't want to bikeshed about this but ARROW_QUERY_ENGINE or something more "obvious" might aid readability, unless we come up with some other name for the project

wesm · 2020-03-13T19:20:44Z

cpp/src/arrow/CMakeLists.txt

@@ -583,6 +583,10 @@ if(ARROW_DATASET)
  add_subdirectory(dataset)
 endif()

+if(ARROW_ENGINE)
+  add_subdirectory(engine)


Anyone have name opinions. Is arrow/query or arrow/query_engine better? More curious about opinions than anything

Some examples

Impala uses exec

MapD uses QueryEngine

Clickhouse seems to be called "Interpreters"

🤷‍♂

wesm · 2020-03-13T19:25:10Z

cpp/src/arrow/engine/CMakeLists.txt

+endforeach()
+
+# Adding unit tests part of the "engine" portion of the test suite
+function(ADD_ARROW_ENGINE_TEST REL_TEST_NAME)


Note to self and team, we might want to make a helper function to reduce boilerplate for component-specific unit tests

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

wesm · 2020-03-13T19:25:56Z

cpp/src/arrow/engine/CMakeLists.txt

+
+add_arrow_engine_test(catalog_test PREFIX arrow-engine)
+add_arrow_engine_test(expression_test PREFIX arrow-engine)
+add_arrow_engine_test(logical_plan_test PREFIX arrow-engine)


Nit: combine these into a single executable (until it can be shown that splitting them up improves parallel test performance)?

wesm · 2020-03-13T19:31:09Z

cpp/src/arrow/engine/catalog.h

+  class ARROW_EN_EXPORT Entry {
+   public:
+    Entry(std::shared_ptr<dataset::Dataset> dataset, std::string name);
+    Entry(std::shared_ptr<Table> table, std::string name);


Nit: name first?

wesm · 2020-03-13T22:24:56Z

cpp/src/arrow/util/compare.h

@@ -22,6 +22,7 @@
 #include <utility>

 #include "arrow/util/macros.h"
+#include "arrow/util/visibility.h"


IWYU or...?

cpp/src/arrow/testing/gmock.h

wesm · 2020-03-13T22:27:08Z

cpp/src/arrow/engine/logical_plan.h

+  /// \brief References a field by index.
+  ResultExpr Field(const std::shared_ptr<Expr>& input, int field_index);
+  /// \brief References a field by name.
+  ResultExpr Field(const std::shared_ptr<Expr>& input, const std::string& field_name);


Echoing comments about nested column paths

wesm · 2020-03-13T22:27:39Z

cpp/src/arrow/engine/logical_plan.h

@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one


I'm fine with this file being somewhat WIP, a lot of stuff needs to be fleshed out and I expect refactoring. There's some other stuff that needs to be resolved either here or in follow up PRs

wesm · 2020-03-13T22:29:37Z

cpp/src/arrow/engine/expression.cc

+             " should not be have a table shape");
+    // TODO(fsaintjacques): better name handling. Callers should be able to
+    // pass a vector of names.
+    fields.push_back(field("expr", expr_type.type()));


Yeah I think names need to be a property of the expr

bkietz · 2020-03-16T18:52:00Z

cpp/src/arrow/engine/logical_plan.h

+
+class ARROW_EN_EXPORT LogicalPlanBuilder {
+ public:
+  using ResultExpr = Result<std::shared_ptr<Expr>>;


Suggested change

using ResultExpr = Result<std::shared_ptr<Expr>>;

using MaybeExpr = Result<std::shared_ptr<Expr>>;

bkietz · 2020-03-17T13:41:58Z

cpp/src/arrow/engine/expression.h

+}
+
+template <typename E>
+bool IsA(const std::shared_ptr<Expr>& expr) {


Since the const& overloads are enabled, it'd be best to forward that to this overload with a trailing return type:

Suggested change

bool IsA(const std::shared_ptr<Expr>& expr) {

auto IsA(const std::shared_ptr<Expr>& expr) -> decltype(IsA<E>(*expr)) {

bkietz · 2020-03-17T13:46:25Z

cpp/src/arrow/engine/expression.h

+  if (!expr) return false;
+  return IsA<E>(*expr);
+}
+


Suggested change

template <typename E, typename Enable = decltype(IsA<E>(std::declval<const Expr&>()))>

const E* DynCast(const Expr& expr) {

return IsA<E>(expr) ? &expr : NULLPTR;

}

template <typename E, typename Enable = decltype(IsA<E>(std::declval<const Expr&>()))>

const E* DynCast(const std::shared_ptr<Expr>& expr) {

return IsA<E>(expr) ? expr.get() : NULLPTR;

}

kszucs · 2020-04-29T07:22:37Z

@fsaintjacques where do we stand on this?

wesm · 2020-04-29T14:51:25Z

I'm going to review this again in the near future. I don't think this is blocking anything at the moment?

emkornfield · 2020-08-27T03:47:28Z

@wesm @fsaintjacques @kszucs do you want to keep this PR open or resurrect it at some point in the future?

kszucs · 2020-08-27T09:30:32Z

I'd assume so, but not sure when are we going to have the time for it.

wesm · 2020-08-27T12:33:25Z

Will close for now until there is bandwidth to make a push on this project.

fsaintjacques commented Mar 1, 2020

View reviewed changes

andygrove reviewed Mar 1, 2020

View reviewed changes

kszucs reviewed Mar 2, 2020

View reviewed changes

cpp/src/arrow/engine/catalog.h Outdated Show resolved Hide resolved

pitrou reviewed Mar 2, 2020

View reviewed changes

wesm self-requested a review March 2, 2020 17:02

kszucs reviewed Mar 3, 2020

View reviewed changes

cpp/src/arrow/engine/catalog.h Outdated Show resolved Hide resolved

bkietz self-requested a review March 3, 2020 19:11

bkietz requested changes Mar 3, 2020

View reviewed changes

fsaintjacques force-pushed the ARROW-7878-logical-plan branch from 42e014d to 60a4887 Compare March 9, 2020 13:55

fsaintjacques commented Mar 11, 2020

View reviewed changes

fsaintjacques force-pushed the ARROW-7878-logical-plan branch 10 times, most recently from c956347 to d1f8c6d Compare March 13, 2020 11:48

wesm reviewed Mar 13, 2020

View reviewed changes

fsaintjacques added 15 commits March 16, 2020 11:00

Add key in Catalog::Entry

77c5bf9

Update

1b59312

Add projection operator

92bf425

Removes Table support from Catalog::Entry

8ea5ddd

Add more comment

8b47016

Move compare enums into a single enum

236ff3c

FieldRef should preserve the input

52d5a9b

Add explicit libarrow_engine.so

7d78abe

Fix msvc

6fbac47

Add custom ARROW_EN_EXPORT for engine

4a47809

Fix constant type

5679f83

Make msvc happy

a9fc553

Make inner class exportable

f35c500

Try msvc

369af82

Add Aggregate operators with Count and Sum

0423241

fsaintjacques force-pushed the ARROW-7878-logical-plan branch from 254677a to 0423241 Compare March 16, 2020 15:00

fsaintjacques added 2 commits March 16, 2020 14:00

Add Compare expressions to LogicalPlanBuilder

24675c6

Add missing forward

ef10404

bkietz reviewed Mar 16, 2020

View reviewed changes

Add IsA<Expr>

cf58f34

bkietz requested changes Mar 17, 2020

View reviewed changes

wesm force-pushed the master branch from 5fe5b88 to aa55967 Compare April 19, 2020 22:46

kszucs force-pushed the master branch from 1b71ca7 to 5093b80 Compare April 20, 2020 19:21

wesm closed this Aug 27, 2020

asfimport mentioned this pull request Oct 8, 2021

[C++] Implement LogicalPlan and LogicalPlanBuilder #24103

Closed

		@@ -0,0 +1,31 @@
		// Licensed to the Apache Software Foundation (ASF) under one

		@@ -173,6 +173,8 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")

		define_option(ARROW_DATASET "Build the Arrow Dataset Modules" OFF)

		define_option(ARROW_ENGINE "Build the Arrow Query Engine Modules" OFF)

		@@ -0,0 +1,127 @@
		// Licensed to the Apache Software Foundation (ASF) under one

	using ResultExpr = Result<std::shared_ptr<Expr>>;
	using MaybeExpr = Result<std::shared_ptr<Expr>>;

	bool IsA(const std::shared_ptr<Expr>& expr) {
	auto IsA(const std::shared_ptr<Expr>& expr) -> decltype(IsA<E>(*expr)) {

+template <typename E, typename Enable = decltype(IsA<E>(std::declval<const Expr&>()))>
+const E* DynCast(const Expr& expr) {
+  return IsA<E>(expr) ? &expr : NULLPTR;
+}
+template <typename E, typename Enable = decltype(IsA<E>(std::declval<const Expr&>()))>
+const E* DynCast(const std::shared_ptr<Expr>& expr) {
+  return IsA<E>(expr) ? expr.get() : NULLPTR;
+}

ARROW-7878: [C++][Compute] Draft LogicalPlan classes #6506

ARROW-7878: [C++][Compute] Draft LogicalPlan classes #6506

Uh oh!

Conversation

fsaintjacques commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Mar 1, 2020

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsaintjacques Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kszucs commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkietz commented Mar 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kszucs commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fsaintjacques commented Mar 11, 2020 • edited by kszucs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kszucs commented Mar 11, 2020

fsaintjacques commented Feb 28, 2020 •

edited

Loading

fsaintjacques Mar 11, 2020 •

edited

Loading

kszucs commented Mar 5, 2020 •

edited

Loading

kszucs commented Mar 11, 2020 •

edited

Loading

fsaintjacques commented Mar 11, 2020 •

edited by kszucs

Loading

fsaintjacques commented Mar 13, 2020 •

edited

Loading