Initial commit of substrait-cpp #1

chaojun-zhang · 2022-10-10T03:14:39Z

This PR is an Initial commit of substrait-cpp which includes following features:

parse substrait extension yaml file as Type.h, Function.h and Extension.h
lookup function variant by types

chaojun-zhang · 2022-10-10T03:18:18Z

@jacques-n, please help to review this PR, thanks.

CLAassistant · 2022-10-10T05:30:57Z

All committers have signed the CLA.

westonpace

I've put in a few specific comments but I think there are larger overall questions. I don't know if these all need to be answered now but it's probably worth discussion.

Is there a particular style guide you would like to follow? I'm used to Arrow-C++ which follows the Google C++ style guide but this code is quite divergent from that so do you have an alternative style guide you can point new contributors to?
Is your goal to create a library used by consumers? by producers? or both?
What operating systems and architectures are you planning on supporting directly? What sorts of targets do you hope will eventually be supported by the community as a whole?
What kind of philosophy do you hope to take for exception handling. I see in some places that you are throwing exceptions. Some libraries will go out of their way to try and avoid exposing exceptions for easier compatibility. Do you have a strong opinion on this?
What sort of philosophy do you plan on taking for encapsulating the standard library? Right now you are including in Type.h for example which puts a rather large standard include in your public API. Do you want to eventually hide these things in implementation files as much as possible?

westonpace · 2022-10-25T16:52:34Z

CMakeLists.txt

+  set(FILESYSTEM "stdc++fs")
+
+  # Ensure we have gcc at least 8+.
+  if(CMAKE_CXX_COMPILER_VERSION LESS 8.0)


Any particular reason to target this version? I think this is probably a good cutoff (allows for C++17) but I was just curious.

westonpace · 2022-10-25T16:52:48Z

CMakeLists.txt

+  # Ensure we have gcc at least 8+.
+  if(CMAKE_CXX_COMPILER_VERSION LESS 8.0)
+    message(
+      FATAL_ERROR "Substrait-CPP requires gcc > 8. Found ${CMAKE_CXX_COMPILER_VERSION}")


Suggested change

FATAL_ERROR "Substrait-CPP requires gcc > 8. Found ${CMAKE_CXX_COMPILER_VERSION}")

FATAL_ERROR "Substrait-CPP requires gcc >= 8. Found ${CMAKE_CXX_COMPILER_VERSION}")

westonpace · 2022-10-25T16:53:21Z

CMakeLists.txt

+      FATAL_ERROR "Substrait-CPP requires gcc > 8. Found ${CMAKE_CXX_COMPILER_VERSION}")
+  endif()
+else()
+  set(FILESYSTEM "")


What other OS' do you plan on supporting?

westonpace · 2022-10-25T16:55:35Z

src/CMakeLists.txt

+set(proto_directory ${CMAKE_SOURCE_DIR}/third_party/substrait/proto)
+set(substrait_proto_directory ${proto_directory}/substrait)
+set(PROTO_OUTPUT_DIR "${CMAKE_CURRENT_SOURCE_DIR}/proto/")


Minor nit: the casing here seems inconsistent proto_directory vs. PROTO_OUTPUT_DIR, can we be more consistent?

westonpace · 2022-10-25T16:59:24Z

src/Extension.cpp

+
+using namespace io::substrait;
+
+static bool decodeFunctionVariant(const Node &node, FunctionVariant &function) {


Minor nit: I think anonymous namespaces are generally preferred over static functions.

moved to anonymous namespaces

westonpace · 2022-10-25T17:01:44Z

src/Extension.cpp

+
+#include "Extension.h"
+
+namespace YAML {


Suggested change

namespace YAML {

namespace YAML {

This is a bit peculiar to me. Why namespace YAML? It's not referenced or used anywhere else.

I believe this is used here to establish unqualified access to the members of the YAML namespace, which are declared by yaml-cpp. This is very bad for readability, could we have one consistent namespace for this project (namespace substrait_cpp, maybe) then refer to members of other namespaces by their qualified names (YAML::Node, for example)?

westonpace · 2022-10-25T17:05:55Z

src/FunctionMapping.h

+#include "memory"
+#include "unordered_map"
+#include "vector"


I'm used to seeing the <unordered_map> style used for standard includes.

westonpace · 2022-10-25T17:10:05Z

CMakeLists.txt

+find_package(Protobuf REQUIRED)
+include_directories(${PROTOBUF_INCLUDE_DIRS})


I'm not a cmake expert but why do we do find_package for protobuf but not for yaml-cpp?

because we have to use 'protoc' command to compile substrait protocol files before we compile this project.

westonpace · 2022-10-25T17:17:32Z

src/Function.h

+  virtual const bool isWildcardType() const { return false; };
+
+  virtual const bool isValueArgument() const { return false; }


I'm not sure there is much value in returning a const bool

westonpace · 2022-10-25T17:18:56Z

src/Function.h

+struct EnumArgument : public FunctionArgument {
+  bool required;
+  bool const isRequired() const override { return required; }
+
+  const std::string toTypeString() const override {
+    return required ? "req" : "opt";
+  }
+};


This is probably going to be out of date once substrait-io/substrait#342 merges. Though I'm not sure there is much that can be done about that. Have you taken a look at substrait-io/substrait#342 ?

Yes, I noticed this PR, I will rebase the substrait submodule once it merged.

pitrou · 2022-10-25T17:33:49Z

CMakeLists.txt

+find_package(Protobuf REQUIRED)
+include_directories(${PROTOBUF_INCLUDE_DIRS})
+
+# GCC needs to link a library to enable std::filesystem.


Why is std::filesystem needed for a substrait library?

pitrou · 2022-10-25T17:33:56Z

CMakeLists.txt

+if("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
+  set(FILESYSTEM "stdc++fs")
+
+  # Ensure we have gcc at least 8+.


pitrou · 2022-10-25T17:38:19Z

src/Type.h

+#pragma once
+
+#include "proto/substrait/algebra.pb.h"
+#include "proto/substrait/type.pb.h"


It's not a good idea to expose protobuf headers in this library's headers, IMHO.

do you have any suggestions?

The usual solution is to not expose any protobuf-generated types in your own headers. So define your own C++ classes and enums for any types that might have an equivalent in the protobuf definitions...

This is the solution we use in Parquet (with Thrift), Arrow (with flatbuffers), Arrow Flight (with protobuf).

(I'll add that even with that, protobuf still has issues in non-trivial scenarios: substrait-io/substrait#249 ; but it will have less of them)

Sounds good, I will refactor to use our own type enums instead of protobuf based type enums.

bkietz · 2022-10-26T16:31:49Z

src/Extension.cpp

+
+#include "Extension.h"
+
+namespace YAML {


I believe this is used here to establish unqualified access to the members of the YAML namespace, which are declared by yaml-cpp. This is very bad for readability, could we have one consistent namespace for this project (namespace substrait_cpp, maybe) then refer to members of other namespaces by their qualified names (YAML::Node, for example)?

bkietz · 2022-10-26T16:34:43Z

src/Extension.cpp

+
+namespace YAML {
+
+using namespace io::substrait;


Suggested change

using namespace io::substrait;

using namespace io::substrait;

Similar to my other comment https://github.com/substrait-io/substrait-cpp/pull/1/files#r1004739159 I recommend against using namespace. This is broadly considered bad practice by several style guides https://isocpp.org/wiki/faq/coding-standards#using-namespace-std

bkietz · 2022-10-26T16:37:47Z

src/Type.h

+class UsedDefinedType : public TypeBase<TypeKind ::kUserDefined> {
+public:
+  UsedDefinedType(const std::string &value) : value_(value) {}


Suggested change

class UsedDefinedType : public TypeBase<TypeKind ::kUserDefined> {

public:

UsedDefinedType(const std::string &value) : value_(value) {}

class UserDefinedType : public TypeBase<TypeKind ::kUserDefined> {

public:

UserDefinedType(const std::string &value) : value_(value) {}

bkietz · 2022-10-26T16:41:30Z

src/Type.h

+#define SCALAR_ACCESSOR(KIND)                                                  \
+  std::shared_ptr<const ScalarType<TypeKind::KIND>> KIND()
+
+SCALAR_ACCESSOR(kBool);


This results in functions declarations like:

std::shared_ptr<const ScalarType<TypeKind::kBool>> kBool();

which will surprise users who expect the kThing naming convention to refer only to enum members. In this case I think we're not saving a lot of boilerplate by using a macro and it'd be better to write the declarations without using one:

Suggested change

#define SCALAR_ACCESSOR(KIND) \

std::shared_ptr<const ScalarType<TypeKind::KIND>> KIND()

SCALAR_ACCESSOR(kBool);

std::shared_ptr<const ScalarType<TypeKind::kBool>> bool();

bkietz · 2022-10-26T16:42:33Z

src/FunctionMapping.h

+  virtual const FunctionMap scalaMapping() const {
+    static const FunctionMap scalaFunctionMap{};
+    return scalaFunctionMap;


I think your intent here was to return a constant reference to a static instance, but without the & you'll be copying it instead. That's not a problem for the default mapping, but as mappings get larger it'll add significant overhead which is unnecessary unless you intend for FunctionMappings to be mutable after construction (in which case they'd need to construct a new FunctionMap on every call).

Suggested change

virtual const FunctionMap scalaMapping() const {

static const FunctionMap scalaFunctionMap{};

return scalaFunctionMap;

virtual const FunctionMap& scalarMapping() const {

static const FunctionMap scalarFunctionMap{};

return scalarFunctionMap;

bkietz · 2022-10-26T16:52:31Z

src/Function.h

+  std::optional<int> max;
+};
+
+struct FunctionVariant {


IIUC, this corresponds to an element of simple_extensions_schema.yaml::$defs.scalarFunction.impls. To reduce confusion, perhaps this could be renamed to FunctionImpl?

I saw substrait-java also use FunctionVariant

bkietz · 2022-10-26T17:08:31Z

src/Extension.cpp

+  const std::string absolute_path = __FILE__;
+  auto const pos = absolute_path.find_last_of('/');
+  return absolute_path.substr(0, pos) + "/../third_party/substrait/extensions/";


I think relying on a macro in this fashion is extremely fragile. The standard doesn't give guarantees on this macro's precise value https://eel.is/c++draft/cpp#predefined-1.4 (could be relative to the working directory of the compiler, could fail to include any directory information at all, on msvc the directory separators are \, ...).

Moreover, this approach assumes that the path represented by getSubstraitExtensionAbsolutePath() will always refer to a valid directory; if the library were built then the repository deleted, or if the library were copied to a different machine without the rest of the files in the repo then that will break this function.

As an alternative, what if we got this path from an environment variable? Or required it to be configured by users via getSubstraitExtensionAbsolutePath(std::string)?

bkietz · 2022-10-26T17:20:31Z

src/Extension.h

+class Extension {
+public:
+  /// Deserialize default substrait extension.
+  static std::shared_ptr<Extension> load();


yaml-cpp will throw an exception in case the file cannot be found or parsing fails. Is it your intention to defer such an exception? If so, I'd say it's important to document which functions can throw and under which circumstances. In light of the fact that many c++ projects avoid usage of exceptions, you might prefer to catch yaml-cpp's exceptions and report errors through a different channel (for example, something analagous to std::expected).

Got it ,do you have any suggestion for this exception handling?

chaojun-zhang · 2022-10-28T01:44:21Z

chaojun-zhang · 2022-10-28T15:08:32Z

I've put in a few specific comments but I think there are larger overall questions. I don't know if these all need to be answered now but it's probably worth discussion.

Is there a particular style guide you would like to follow? I'm used to Arrow-C++ which follows the Google C++ style guide but this code is quite divergent from that so do you have an alternative style guide you can point new contributors to?

uploaded clang-format in this PR.

Is your goal to create a library used by consumers? by producers? or both?

The long term would be both just like substrait-java does, and the next step is to create relation & expression CPP classes to represent PROTOBUF based types.

What operating systems and architectures are you planning on supporting directly? What sorts of targets do you hope will eventually be supported by the community as a whole?

I hope this tiny project should support mainstream OS like ubuntu, macos etc.

What kind of philosophy do you hope to take for exception handling. I see in some places that you are throwing exceptions. Some libraries will go out of their way to try and avoid exposing exceptions for easier compatibility. Do you have a strong opinion on this?

Please have a look at exception handling here

What sort of philosophy do you plan on taking for encapsulating the standard library? Right now you are including in Type.h for example which puts a rather large standard include in your public API. Do you want to eventually hide these things in implementation files as much as possible?

Sure, I have move public API into include folder

Init commit of substrait-cpp

chaojun-zhang force-pushed the main branch from 47268d0 to afff499 Compare October 11, 2022 14:20

westonpace reviewed Oct 25, 2022

View reviewed changes

pitrou reviewed Oct 25, 2022

View reviewed changes

bkietz suggested changes Oct 26, 2022

View reviewed changes

substrait yaml file parser

780f6c1

chaojun-zhang force-pushed the main branch from b5a88fe to 780f6c1 Compare October 28, 2022 01:41

chaojun-zhang closed this Oct 28, 2022

chaojun-zhang mentioned this pull request Oct 29, 2022

Initial commit of substrait-cpp #2

Merged

chaojun-zhang referenced this pull request in chaojun-zhang/substrait-cpp Feb 8, 2023

Merge pull request #1 from chaojun-zhang/main

253bacd

Init commit of substrait-cpp

	FATAL_ERROR "Substrait-CPP requires gcc > 8. Found ${CMAKE_CXX_COMPILER_VERSION}")
	FATAL_ERROR "Substrait-CPP requires gcc >= 8. Found ${CMAKE_CXX_COMPILER_VERSION}")


		using namespace io::substrait;

		static bool decodeFunctionVariant(const Node &node, FunctionVariant &function) {

		find_package(Protobuf REQUIRED)
		include_directories(${PROTOBUF_INCLUDE_DIRS})

		virtual const bool isWildcardType() const { return false; };

		virtual const bool isValueArgument() const { return false; }

Initial commit of substrait-cpp #1

Initial commit of substrait-cpp #1

Conversation

chaojun-zhang commented Oct 10, 2022 • edited Loading

chaojun-zhang commented Oct 10, 2022

CLAassistant commented Oct 10, 2022 • edited Loading

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaojun-zhang commented Oct 28, 2022

chaojun-zhang commented Oct 28, 2022

chaojun-zhang commented Oct 10, 2022 •

edited

Loading

CLAassistant commented Oct 10, 2022 •

edited

Loading