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

feat: relations and expressions binary output for the textplan parser #63

Merged
merged 4 commits into from
Jun 1, 2023

Conversation

EpsilonPrime
Copy link
Member

@EpsilonPrime EpsilonPrime commented May 31, 2023

* Relations are now interconnected as defined by pipelines.
* Expressions are now emitted to binary plans.
* HasErrors matcher now explains the difference in errors.

@@ -117,7 +117,15 @@ std::vector<TestCase> getTestCases() {
WhenSerialized(EqSquashingWhitespace(R"(pipelines {
myself -> a;
b -> myself;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think that a pipeline referring to a node that isn't defined would be an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed issue #64.

AsBinaryPlan(Partially(EqualsProto<::substrait::proto::Plan>(
R"(relations { root { input { project {
expressions { cast { type { i32 {} } input { literal { i8: 123 } } } }
expressions { cast { type { i64 {} } input { cast { type { i32 {} } input { literal { i8: 123 } } } } } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that you did not try to be clever and collapse these casts. We can save that for other tools.

src/substrait/textplan/parser/tests/TextPlanParserTest.cpp Outdated Show resolved Hide resolved
}

join relation join {
//type JOIN_TYPE_UNSPECIFIED;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an example showing that we can support comments in splans?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was waiting for support, I've replaced this with the future syntax which is currently just getting swallowed.

@EpsilonPrime EpsilonPrime changed the title Parser part4 feat: relations and expressions binary output for the textplan parser May 31, 2023
        * Relations are now interconnected as defined by pipelines.
        * Expressions are now emitted to binary plans.
        * HasErrors matcher now explains the difference in errors.
Comment on lines +447 to +448
// The type isn't one we expected bail.
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might consider some kind of UNREACHABLE macro which does nothing in release mode but throws an exception (or asserts with the line number) in debug mode.

case RelationType::kJoin:
if (relationData->newPipelines.size() == kBinaryRelationInputCount) {
auto leftRelationData = ANY_CAST(
std::shared_ptr<RelationData>, relationData->newPipelines[0]->blob);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this means...

read1 -> project1 -> join1
read2 -> project2 -> join1

In this case project1 will be the left and project2 will be the right?

@westonpace westonpace merged commit 1605e89 into substrait-io:main Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants