Skip to content

feat: add proto converters and visitor, both incomplete atm #4

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add proto converters and visitor, both incomplete atm #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
206ac27
feat: add proto converters and visitor
Dec 6, 2022
20e6bcc
fix: use consistent naming for relations
ashvina Dec 9, 2022
52b365e
fix: match rel type and resolve review comment
ashvina Dec 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/Substrait.Core/Expression/FieldReference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
namespace Substrait.Expression
{
/// <summary>
/// The FIELD_REFERENCE relational expression, <see cref="Protobuf.Expression.Types.FieldReference"/>
/// </summary>
public class FieldReference : RelExpression
{
}
}
14 changes: 14 additions & 0 deletions src/Substrait.Core/Expression/RelExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
using Substrait.Relation;

namespace Substrait.Expression
{
/// <summary>
/// Base type for all relational expressions, <see cref="Protobuf.Expression"/>
/// </summary>
abstract public class RelExpression
Copy link
Member

Choose a reason for hiding this comment

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

Why RelExpression and not just Expression? An expression is not an instance or a specialization of a Rel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry for the confusion. I called it so to mean relational expression. I didn't use Expression or Rex because it was hurting code readability. Like my earlier comment, I can fix it later next week or in a subsequent PR if this one is merged. Thanks !

{
public virtual void Accept(SubstraitRelVisitor visitor)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be abstract? Also, is SubstraitRelVisitor intended to visit both relations and expressions? Or should this be SubstraitExpressionVisitor?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 on separation of visitor for relations and expressions (Java implementation can serve as a model).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, separation would be more meaningful. If it is ok with you both, I'd like to fix this in a subsequent PR.

{
}
}
}
154 changes: 154 additions & 0 deletions src/Substrait.Core/ProtoConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
using Substrait.Expression;
using Substrait.Protobuf;
using Substrait.Relation;
using System.Collections.Immutable;
using ProtoRel = Substrait.Protobuf.Rel;
using ProtoRex = Substrait.Protobuf.Expression;
using Rel = Substrait.Relation.Rel;

namespace Substrait.Core
{
/// <summary>
/// Utility to convert Substrait protobuf messages to/from charp domain-objects.
/// </summary>
Copy link

Choose a reason for hiding this comment

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

Can you please more details on the motivation to add a Substrait.Relation.Rel mappings for the protobuf objects? I see some of it in the previous PR, trying to learn more.

Copy link
Member

Choose a reason for hiding this comment

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

One very useful feature is that Rel has a property Inputs:

    public IList<Rel> Inputs { get; init; } = new List<Rel>();

This can be used, for example, to do a depth-first traversal of the relations tree. However, in the protobuf objects, there is no such property. For example, a join relation has left and right properties and a filter relation has an input property.

Copy link

Choose a reason for hiding this comment

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

Ok.
Is the intention for C# users to use these objects to interface with substrait instead of the protofbuf objects when it is fully ready?

public class ProtoConverter
Copy link
Collaborator

Choose a reason for hiding this comment

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

One possible recommendation for the ProtoConverter to split work in multiple small PRs while at the same time making committed code self-contained is to divide the work across operator boundaries, e.g., take scan operator and translate all its existing contents from proto into internal representation, including unit tests to validate the translation using a few examples (maybe consisting of a single op).

Said that, it might still be fine to push this PR since we are in very early stages of the library development (I'd like to know what @westonpace thinks too) but maybe add some comments in the files with a TODO/warning note so there is a clear understanding that translation has some significant gaps (as it is already pointed out in the PR title)? Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think a comment, or even throwing a NotImplementedException is probably fine. But I think whatever feels natural is fine. This seems like a rather arbitrary swath of functionality in this PR but, on the other hand, it is enough to start sparking some initial feedback.

I've found that is quite quick to get to 80% coverage and then the last 20% ends up being quite tricky. For example, user defined types and extension relations will probably require more thought. Going operator by operator can be challenging because, for example, a project relation needs an expression which could be anything (and, as mentioned, user defined type expressions might come quite a bit later).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was not implying that conversion should be complete (maybe I did not express that clearly enough); as you mention, that final 20% is usually hard and would slow down the whole development. I was thinking more about making translation less arbitrary and support boundaries a bit more clear, which you also referred to. But it's not a very strong opinion, overall I'm happy that we continue making progress one way or another.

Copy link
Contributor Author

@ashvina ashvina Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jcamachor, I see value in your suggestion, particularly from tracking and reviews. Anyone who checks out this commit would know that what features will work. I started it like that with just inputs to start with. But I can pay more attention going forward. If you think this commit creates confusion, I can fix it later next week.

@westonpace, as you said my intention is to gather early feedback and let others jump in if feasible.

Thanks both !

{
/// <summary>
/// Create domain object from a protobuf encoded message
/// </summary>
/// <param name="protoRel">Relation encoded as protobuf message</param>
/// <returns><see cref="Rel"/> corresponding to message's relation</returns>
public Rel ToRel(ProtoRel protoRel)
{
var relType = protoRel.RelTypeCase;

return relType switch
{
ProtoRel.RelTypeOneofCase.Aggregate => CreateAgg(protoRel.Aggregate),
ProtoRel.RelTypeOneofCase.Cross => CreateCross(protoRel.Cross),
ProtoRel.RelTypeOneofCase.Fetch => CreateFetch(protoRel.Fetch),
ProtoRel.RelTypeOneofCase.Filter => CreateFilter(protoRel.Filter),
ProtoRel.RelTypeOneofCase.Join => CreateJoin(protoRel.Join),
ProtoRel.RelTypeOneofCase.Project => CreateProject(protoRel.Project),
ProtoRel.RelTypeOneofCase.Read => CreateRead(protoRel.Read),
ProtoRel.RelTypeOneofCase.Sort => CreateSort(protoRel.Sort),
_ => throw new NotImplementedException(relType.ToString()),
};
}

/// <summary>
/// Create domain object specific to relational expression type from protobuf message
/// </summary>
/// <param name="protoRex">Relational expression encoded as protobuf message</param>
/// <returns><see cref="Rel"/> corresponding to message's expression type</returns>
Copy link
Member

Choose a reason for hiding this comment

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

This returns a RelExpression, not a Rel.

private RelExpression ToExpression(ProtoRex protoRex)
{
var rexType = protoRex.RexTypeCase;

return rexType switch
{
ProtoRex.RexTypeOneofCase.Selection => CreateFieldReference(protoRex.Selection),
ProtoRex.RexTypeOneofCase.None => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.Literal => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.ScalarFunction => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.WindowFunction => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.IfThen => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.SwitchExpression => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.SingularOrList => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.MultiOrList => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.Cast => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.Subquery => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.Nested => throw new NotImplementedException(),
ProtoRex.RexTypeOneofCase.Enum => throw new NotImplementedException(),
_ => throw new NotImplementedException(rexType.ToString()),
};
}

private Rel CreateAgg(AggregateRel agg)
{
var input = ToRel(agg.Input);
var inputs = ImmutableList.Create(input);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems there are important parts missing in the translation, e.g., grouping columns or aggregates. Same comment for other operators below (e.g., expressions for join, filter, or project, different type of read rels, etc.)

return new Aggregate()
{
Inputs = inputs,
};
}

private Rel CreateCross(CrossRel cross)
{
var left = ToRel(cross.Left);
var right = ToRel(cross.Right);
var inputs = ImmutableList.Create(left, right);
return new Cross()
{
Inputs = inputs,
};
}

private Rel CreateFetch(FetchRel fetch)
{
var input = ToRel(fetch.Input);
var inputs = ImmutableList.Create(input);
return new Fetch()
{
Count = fetch.Count,
Inputs = inputs,
};
}

private Rel CreateFilter(FilterRel filter)
{
var input = ToRel(filter.Input);
var inputs = ImmutableList.Create(input);
return new Filter()
{
Inputs = inputs,
};
}

private Rel CreateJoin(JoinRel join)
{
var left = ToRel(join.Left);
var right = ToRel(join.Right);
var _ = ToExpression(join.Expression);
Copy link
Member

Choose a reason for hiding this comment

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

This seems a touch arbitrary (e.g. why grab join expression and not the join filter? why not grab a filter's filter expression?) but there's probably no reason to get hung up on it.

var inputs = ImmutableList.Create(left, right);
return new Join()
{
Inputs = inputs,
};
}

private Rel CreateProject(ProjectRel project)
{
var input = ToRel(project.Input);
var inputs = ImmutableList.Create(input);
return new Project()
{
Inputs = inputs,
};
}

private Rel CreateRead(ReadRel read)
{
return new Read()
{
Inputs = ImmutableList<Rel>.Empty,
};
}

private Rel CreateSort(SortRel sort)
{
var input = ToRel(sort.Input);
var inputs = ImmutableList.Create(input);
return new Sort()
{
Inputs = inputs,
};
}

private RelExpression CreateFieldReference(ProtoRex.Types.FieldReference field)
{
return new FieldReference();
}
}
}
9 changes: 9 additions & 0 deletions src/Substrait.Core/Relation/Cross.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
namespace Substrait.Relation
{
/// <summary>
/// The CROSS relational operator representing cartesian product, <see cref="Protobuf.CrossRel"/>
/// </summary>
public class Cross : Rel
{
}
}
1 change: 1 addition & 0 deletions src/Substrait.Core/Relation/Fetch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@
/// </summary>
public class Fetch : Rel
{
public long Count { get; init; }
}
}
58 changes: 57 additions & 1 deletion src/Substrait.Core/Relation/Join.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,65 @@
namespace Substrait.Relation
using Substrait.Expression;
using Substrait.Protobuf;

namespace Substrait.Relation
{
/// <summary>
/// The binary JOIN relational operator, <see cref="Protobuf.JoinRel"/>
/// </summary>
public class Join : Rel
{
public RelExpression? Condition { get; init; }
public RelExpression? PostJoinFilter { get; init; }
public JoinType JoinType { get; init; } = JoinType.UNKNOWN;

public override void Accept(SubstraitRelVisitor visitor)
{
base.Accept(visitor);
Condition?.Accept(visitor);
PostJoinFilter?.Accept(visitor);
}
}

public enum JoinType
{
/// <summary>
/// <see cref="JoinRel.Types.JoinType.Anti"/>
/// </summary>
ANTI,

/// <summary>
/// <see cref="JoinRel.Types.JoinType.Inner"/>
/// </summary>
INNER,

/// <summary>
/// <see cref="JoinRel.Types.JoinType.Left"/>
/// </summary>
LEFT,

/// <summary>
/// <see cref="JoinRel.Types.JoinType.Outer"/>
/// </summary>
OUTER,

/// <summary>
/// <see cref="JoinRel.Types.JoinType.Right"/>
/// </summary>
RIGHT,

/// <summary>
/// <see cref="JoinRel.Types.JoinType.Semi"/>
/// </summary>
SEMI,

/// <summary>
/// <see cref="JoinRel.Types.JoinType.Single"/>
/// </summary>
SINGLE,

/// <summary>
/// <see cref="JoinRel.Types.JoinType.Unspecified"/>
/// </summary>
UNKNOWN
}
}
12 changes: 12 additions & 0 deletions src/Substrait.Core/Relation/Rel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,17 @@
/// </summary>
abstract public class Rel
{
/// <summary>
/// Input relations to this relation
/// </summary>
public IList<Rel> Inputs { get; init; } = new List<Rel>();

public virtual void Accept(SubstraitRelVisitor visitor)
{
foreach (var input in Inputs)
{
input.Accept(visitor);
}
}
}
}
6 changes: 2 additions & 4 deletions src/Substrait.Core/SubstraitRelVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
using Substrait.Relation;

namespace Substrait.Core
namespace Substrait.Relation
{
/// <summary>
/// Visitor to transform, compile, and/or process SQL logical operators represented using Substrait. The visitor has
Expand Down Expand Up @@ -99,7 +97,7 @@ public void Visit(Sort sort)
Fallback(sort);
}

public void Fallback(Rel _)
protected void Fallback(object _)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't Fallback still accept a Rel object rather than generic object?

Copy link
Contributor Author

@ashvina ashvina Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I made it type independent since Fallback needs to accept Rel, or Expression, and in future other types. I dont think adding one fallback method for such types adds any value. WDYT?

{
throw new InvalidOperationException();
}
Expand Down
20 changes: 20 additions & 0 deletions src/Substrait.Lineage/LineageExtractorRelVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
using Substrait.Core;
using Substrait.Relation;
using ProtoRel = Substrait.Protobuf.Rel;

namespace Substrait.Lineage
Copy link
Member

Choose a reason for hiding this comment

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

Hmm...I'm maybe a little confused here. What is the intent of this project? Isn't lineage an implicit property of Rel? I would expect this project would be extending Rel somehow to add lineage information to it (or extracting lineage information into some kind of project-specific classes).

It seems a bit odd that "grabbing lineage for one relation" is a Substrait.Core task but "grabbing lineage for an entire relation tree" is a Substrait.Lineage task. I would think either both are Substrait.Lineage or both are Substrait.Core (with a minor preference for the latter)

{
/// <summary>
/// A concrete implementation of <see cref="SubstraitRelVisitor"/> for processing Substrait relations and extracting
/// lineage information.
/// </summary>
public class LineageExtractorRelVisitor : SubstraitRelVisitor
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 class really needed? I would think the normal behavior for a system would be to do something like:

public void OnMessage(byte[] message)
{
  ProtoRel protoMessage = DecodeMessage(message);
  Rel root = new ProtoConverter().ToRel(protoMessage);
  HandleMessage(root);
}

private void HandleMessage(Rel rel)
{
  // Do some stuff
  LogQuery(rel);
  // Do some other stuff
}

private void LogQuery(Rel rel)
{
  new LoggingVisitor(logger).Visit(rel);
}

In other words, I would expect to convert at the boundaries, then visiting the converted plan as needed. Instead this appears to convert each time I need to visit something.

{
public void Execute(ProtoRel protoRel)
{
ProtoConverter converter = new();
var rel = converter.ToRel(protoRel);
rel.Accept(this);
}
}
}
13 changes: 13 additions & 0 deletions src/Substrait.Lineage/Substrait.Lineage.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net6.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
<ProjectReference Include="..\Substrait.Core\Substrait.Core.csproj" />
</ItemGroup>

</Project>
18 changes: 18 additions & 0 deletions substrait-csharp.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ VisualStudioVersion = 17.3.33027.108
MinimumVisualStudioVersion = 10.0.40219.1
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Substrait.Core", "src\Substrait.Core\Substrait.Core.csproj", "{C33A4305-4A80-4A92-AEF5-FB304D8BA6C7}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Substrait.Lineage", "src\Substrait.Lineage\Substrait.Lineage.csproj", "{365DAF8C-5336-4845-A297-38584F4DB328}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Substrait.Lineage.Tests", "tests\Substrait.Lineage.Tests\Substrait.Lineage.Tests.csproj", "{060D3D24-9395-4C8D-8C0C-95EDF2A86931}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Substrait.Core.Tests", "tests\Substrait.Core.Tests\Substrait.Core.Tests.csproj", "{590A26FC-0099-4A5E-8833-8D6CF5814C51}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand All @@ -15,6 +21,18 @@ Global
{C33A4305-4A80-4A92-AEF5-FB304D8BA6C7}.Debug|Any CPU.Build.0 = Debug|Any CPU
{C33A4305-4A80-4A92-AEF5-FB304D8BA6C7}.Release|Any CPU.ActiveCfg = Release|Any CPU
{C33A4305-4A80-4A92-AEF5-FB304D8BA6C7}.Release|Any CPU.Build.0 = Release|Any CPU
{365DAF8C-5336-4845-A297-38584F4DB328}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{365DAF8C-5336-4845-A297-38584F4DB328}.Debug|Any CPU.Build.0 = Debug|Any CPU
{365DAF8C-5336-4845-A297-38584F4DB328}.Release|Any CPU.ActiveCfg = Release|Any CPU
{365DAF8C-5336-4845-A297-38584F4DB328}.Release|Any CPU.Build.0 = Release|Any CPU
{060D3D24-9395-4C8D-8C0C-95EDF2A86931}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{060D3D24-9395-4C8D-8C0C-95EDF2A86931}.Debug|Any CPU.Build.0 = Debug|Any CPU
{060D3D24-9395-4C8D-8C0C-95EDF2A86931}.Release|Any CPU.ActiveCfg = Release|Any CPU
{060D3D24-9395-4C8D-8C0C-95EDF2A86931}.Release|Any CPU.Build.0 = Release|Any CPU
{590A26FC-0099-4A5E-8833-8D6CF5814C51}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{590A26FC-0099-4A5E-8833-8D6CF5814C51}.Debug|Any CPU.Build.0 = Debug|Any CPU
{590A26FC-0099-4A5E-8833-8D6CF5814C51}.Release|Any CPU.ActiveCfg = Release|Any CPU
{590A26FC-0099-4A5E-8833-8D6CF5814C51}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
27 changes: 27 additions & 0 deletions tests/Substrait.Core.Tests/Relation/ProtoConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
using Substrait.Core;
using Substrait.Protobuf;

namespace Substrait.Relation.Tests
{
[TestClass()]
public class ProtoConverterTests
{
[TestMethod()]
Copy link

Choose a reason for hiding this comment

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

Nit: () not needed.

public void ToRel_FetchRel_Test()
{
FetchRel fetchRel = new FetchRel();
fetchRel.Count = 1; // limit 1
var rel = new Protobuf.Rel()
{
Fetch = fetchRel
};

fetchRel.Input = rel;
Copy link

Choose a reason for hiding this comment

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

This test fails with stack overflow due to this.


ProtoConverter converter = new();
var result = converter.ToRel(rel) as Fetch;
Assert.IsNotNull(result);
Assert.AreEqual(1, result.Count);
}
}
}
Loading