substrait-io / substrait-cpp Goto Github PK

I believe all of our non-antlr dependencies are obtained via submodules. Antlr, on the other hand, is downloaded from github via custom cmake. I think the reasoning behind this discrepancy is that these are the instructions suggested by Antlr itself. Or perhaps it is that Antlr requires java?

Part of me thinks that having these things be consistent will make it easier to understand and maintain for future developers. Is there any particular reason we can't have it as a submodule?

There seem to be several cycles in the dependency graph of CMake targets. This isn't a problem for compiling static libraries, but if I build with cmake -DBUILD_SHARED_LIBS=ON ..., I get the following error (uninteresting parts omitted):

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "substrait_io" of type STATIC_LIBRARY
    depends on "substrait_textplan_converter" (weak)
    depends on "symbol_table" (weak)
    depends on "substrait_textplan_loader" (weak)
    depends on "substrait_textplan_converter" (strong)
    depends on "substrait_textplan_loader" (strong)
  "symbol_table" of type SHARED_LIBRARY
    depends on "substrait_textplan_converter" (weak)
    depends on "substrait_io" (weak)
    depends on "substrait_textplan_loader" (weak)
  "substrait_textplan_converter" of type SHARED_LIBRARY
    depends on "substrait_io" (weak)
    depends on "substrait_textplan_loader" (weak)
    depends on "symbol_table" (weak)
  "substrait_textplan_loader" of type SHARED_LIBRARY
    depends on "symbol_table" (weak)
    depends on "substrait_textplan_converter" (weak)
    depends on "substrait_io" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

I understand that there are reasons not to build shared libraries, but I argue that there are situations where it is useful. (I am working on a project with many top-level binaries that share 99% of the code and compilation and disk usage is a lot better with shared libs, so I hugely prefer that for developing and CI.)

I am currently trying to consume substrait-cpp as a dependency using add_subdirectory. Since I build my top-level project with BUILD_SHARED_LIBS, substrait-cpp is built with the same configuration. For any scenario where that flag is desired or required for the top-level project, support that flag would be great.

Finally, I believe that cyclic dependencies are often an indication for sloppy design or code organization and therefor a good idea to clean up. However, I haven't looked into the cycles yet, so I don't know yet whether there are good reasons to have them or things have just grown this way...

The rules we use in Arrow are:

• If a copy of the variable will never be made then pass const T& if the object is deep and T if the object is shallow (note, I find this deep/shallow thing annoying because things that start out shallow can become deep, so I'm open to improvement)
• If a copy of the variable might be made (or will always be made) then pass T to allow the caller to std::move and avoid the copy
• If the variable itself needs to be modified (e.g. an out parameter) then pass T*. This is different than the google C++ style guide which prefers T&. In Arrow, T* is preferred because it makes it obvious the value is being modified at the call site.

The current implementation of macros such as SUBSTRAIT_FAIL is to return an exception with the location of the exceptions library:

libc++abi: terminating with uncaught exception of type io::substrait::common::SubstraitRuntimeError: Exception: SubstraitRuntimeError
Error Code: INVALID_STATE
Type: system
Reason: Return type from visitExpression with case of 2 was not std::string!
Function: SubstraitException
File: /home/user/projects/substrait-cpp/src/substrait/common/Exceptions.cpp
:Line: 22

Example:

  testDecode<ParameterizedList>(
      "list<list<string?>>",
      [](const std::shared_ptr<const ParameterizedList>& typePtr) {
        ASSERT_FALSE(typePtr->nullable());  // returns true
        ASSERT_FALSE(typePtr->elementType()->nullable());  // returns true
        ASSERT_TRUE(std::dynamic_pointer_cast<const ParameterizedList>(
                        typePtr->elementType())
                        ->elementType()
                        ->nullable());
        ASSERT_EQ(typePtr->signature(), "list<list<str>>");
      });

Right now pipelines are kind of hard to read if there are nodes that have multiple inputs. Let's add a second supported pipeline pattern that is more similar to other query plans. It can use root to leaf ordering. In the design, it will list out a straight pipeline one line after another. If an operator has more than one input, it's children are indented (and subsequent as well) but are prefixed with a dash. For example, a plan that was a two tables read followed each followed by a filter joined together and then aggregated would like this in the old format and the new format:

Old:

pipelines {
  read -> filter -> join
  read2 -> filter2 -> join
  join -> aggregate -> root
}

New Format:

plan {
  aggregate
  join
  -filter
   read
  -filter2
   read2
}

I propose that both formats are supported but only one can be used at a time. When printing, we can request which we want.

Use the symbol table to be the source of truth for extension URI spaces and function numbers for both the parser and the converter. Currently the parser assigns the numbers during the parse. The converter already has the correct numbers (from the binary plan) and stores them in the symbol table. There needs to be one cohesive data structure.

At a minimum we should have CI checks for conventional commit messages as well as running the unit tests.

The current code refers to "variants" in a number of places, typically when talking about multiple implementations of the same function (e.g. add<int32,int32> and add<int64,int64>). Since the spec calls these "implementations" and the spec also has a different concept that it calls "variant" I think it would be good to update the repo changing all instances of "variant" to "implementation".

The converter is tolerant of older plans. This means it will visit the args variant of scalar function arguments which is deprecated. However, this generates compiler warnings. We should suppress those warnings with the appropriate pragmas.

The plan specification allows for a myriad of combinations that are difficult to test exhaustively. Fuzzing could illuminate unexpected combinations such as those caused by unexpected return types encountered by the visitors.

I read the README.md and did as it said, and i got a dir named build-Debug. There are two exe files in substrait-cpp/build-Debug/debug, but i do not know what should i give as a input file, and what the exe file will export. Can someone tell me how to use this project to convert SQL into a plan? I would greatly appreciate your help

Always providing an output type to functions is cumbersome and reduces readability. Investigate using stronger function definitions (such as equal always returning bool if the inputs are non-null) and intelligent defaults (such as add of two like types always returning the same type if not overridden). Override will still be possible and if they are needed they could raise a warning during parsing.

Required:

cmake_format
clang-format
Some kind of spdx licensing header check

Consider:

clang-tidy
iwyu

Doing this will allow CMake to provide the appropriate dependencies instead of needing to run portions of the project in advance to generate files needed to do the analysis.

Physical relations were not part of the basic text plan converter/parser implementation to be implemented later.

Currently the text format uses the sort direction enum as is. However as ascending/desending and nullsfirst/nullslast are two separate concepts it would be more readable to have them separated.

We are about to be pulling in abseil, which is a rather large library, so that we can get int128 support. Assuming we aren't expecting to need all of abseil it might be better to vendor int128 which looks to be fairly separable (and has already been done, at least once, here).

Per the spec, they cannot, even if it could make sense in certain interpretations. Let's discuss and decide what seems most appropriate and then update the implementation to conform.

The text format is missing the ANY keyword.

It would be convenient to be able to run IWYU similarly to how we can run clang format and clang tidy today.

I don't think anyone is using Windows yet but we want this to be portable and this will help to ensure we don't stray too far into Linux-only headers.

extension_leaf, extension_single, and extension_multi currently have no representation in the text format.

This will allow us to add include_directories(src) to the cmake for internal headers (instead of include_directories(.).

ExchangeRel, DdlRel, and WriteRel are not currently representable in a binary substrait plan. After that has been addressed they should be added to the text plan format.

These aren't used by the TPCH queries but they might be useful in some workflows (or with some optimizers).

Currently substrait-cpp use 'substrait' namespace which is conflicts with the namespace defined in substrait proto files. https://github.com/substrait-io/substrait/blob/main/proto/substrait/algebra.proto#L4

Currently it is not an error to have a pipeline node to have a relation definition (or the reverse). This should be at least a warning as the pipeline will not be complete (a text plan with just pipeline information will emit a nearly empty plan).

Currently only named timezones are supported. We should also support timezone offsets such as -0100.

As part of the proposal to add a new text-based language for representing plans add a library that can be used to convert from one to another. Ultimately this new text-based language will replace the JSON-encoded protobuf used today.