-
Notifications
You must be signed in to change notification settings - Fork 93
feat(core): set Substrait version and producer #406
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,3 +12,4 @@ out/** | |
|
|
||
| */bin | ||
| .metals | ||
| .bloop | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,13 +2,21 @@ | |
|
|
||
| import io.substrait.proto.AdvancedExtension; | ||
| import io.substrait.relation.Rel; | ||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.jar.Attributes.Name; | ||
| import java.util.jar.Manifest; | ||
| import org.immutables.value.Value; | ||
|
|
||
| @Value.Immutable | ||
| public abstract class Plan { | ||
|
|
||
| @Value.Default | ||
| public Version getVersion() { | ||
| return Version.DEFAULT_VERSION; | ||
| } | ||
|
|
||
| public abstract List<Root> getRoots(); | ||
|
|
||
| public abstract List<String> getExpectedTypeUrls(); | ||
|
|
@@ -19,6 +27,52 @@ public static ImmutablePlan.Builder builder() { | |
| return ImmutablePlan.builder(); | ||
| } | ||
|
|
||
| @Value.Immutable | ||
| public abstract static class Version { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One small thing I would suggest here is splitting the general concept of the Version POJO from the substrait-java specific Version that we want to create. That is, other java based systems might use the POJOs to manipulate Substrait plans and might want to set their own versions, which gets a bit muddied if we include defaults in the definition IMO. What do you think about something like: @Value.Immutable
public abstract static class Version {
abstract int getMajor();
abstract int getMinor();
abstract int getPatch();
abstract Optional<String> getGitHash();
abstract Optional<String> getProducer();
public static ImmutableVersion.Builder builder() {
return ImmutableVersion.builder();
}
}
public static final Version SUBSTRAIT_JAVA;
static {
final String[] VERSION_COMPONENTS = loadVersion();
SUBSTRAIT_JAVA =
Version.builder()
.minor(Integer.parseInt(VERSION_COMPONENTS[1]))
.patch(Integer.parseInt(VERSION_COMPONENTS[2]))
.producer("substrait-java")
.build();
}which keeps the POJO object generic in that it models any Version, but we provide a pre-populated Version for folks that want to use it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can change that sure. I did set a default mainly because the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed an update to this code: |
||
| public static final Version DEFAULT_VERSION; | ||
|
|
||
| static { | ||
| final String[] versionComponents = loadVersion(); | ||
| DEFAULT_VERSION = | ||
| ImmutableVersion.builder() | ||
| .major(Integer.parseInt(versionComponents[0])) | ||
| .minor(Integer.parseInt(versionComponents[1])) | ||
| .patch(Integer.parseInt(versionComponents[2])) | ||
| .producer(Optional.of("substrait-java")) | ||
| .build(); | ||
| } | ||
|
|
||
| public abstract int getMajor(); | ||
|
|
||
| public abstract int getMinor(); | ||
|
|
||
| public abstract int getPatch(); | ||
|
|
||
| public abstract Optional<String> getGitHash(); | ||
|
|
||
| public abstract Optional<String> getProducer(); | ||
|
|
||
| private static String[] loadVersion() { | ||
| // load the specification version from the JAR manifest | ||
| String specificationVersion = Version.class.getPackage().getSpecificationVersion(); | ||
|
|
||
| // load the manifest directly from the classpath if the specification version is null which is | ||
| // the case if the Version class is not in a JAR, e.g. during the Gradle build | ||
| if (specificationVersion == null) { | ||
| try { | ||
| Manifest manifest = | ||
| new Manifest( | ||
| Version.class.getClassLoader().getResourceAsStream("META-INF/MANIFEST.MF")); | ||
| specificationVersion = manifest.getMainAttributes().getValue(Name.SPECIFICATION_VERSION); | ||
| } catch (IOException e) { | ||
| throw new IllegalStateException("Could not load version from manifest", e); | ||
| } | ||
| } | ||
|
|
||
| return specificationVersion.split("\\."); | ||
| } | ||
| } | ||
|
|
||
| @Value.Immutable | ||
| public abstract static class Root { | ||
| public abstract Rel getInput(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package io.substrait.relation; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
|
||
| import io.substrait.plan.Plan.Version; | ||
| import java.util.Optional; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class SpecVersionTest { | ||
| @Test | ||
| public void testSubstraitVersionDefaultValues() { | ||
| Version version = Version.DEFAULT_VERSION; | ||
|
|
||
| assertNotNull(version.getMajor()); | ||
| assertNotNull(version.getMinor()); | ||
| assertNotNull(version.getPatch()); | ||
|
|
||
| assertEquals(Optional.of("substrait-java"), version.getProducer()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| package io.substrait.isthmus; | ||
|
|
||
| import com.google.common.annotations.VisibleForTesting; | ||
| import io.substrait.extension.ExtensionCollector; | ||
| import io.substrait.plan.ImmutablePlan.Builder; | ||
| import io.substrait.plan.ImmutableVersion; | ||
| import io.substrait.plan.Plan.Version; | ||
| import io.substrait.plan.PlanProtoConverter; | ||
| import io.substrait.proto.Plan; | ||
| import io.substrait.proto.PlanRel; | ||
| import io.substrait.relation.RelProtoConverter; | ||
| import java.util.List; | ||
| import org.apache.calcite.plan.hep.HepPlanner; | ||
| import org.apache.calcite.plan.hep.HepProgram; | ||
|
|
@@ -56,22 +57,18 @@ List<RelRoot> sqlToRelNode(String sql, List<String> tables) throws SqlParseExcep | |
|
|
||
| private Plan executeInner(String sql, SqlValidator validator, Prepare.CatalogReader catalogReader) | ||
| throws SqlParseException { | ||
| var plan = Plan.newBuilder(); | ||
| ExtensionCollector functionCollector = new ExtensionCollector(); | ||
| var relProtoConverter = new RelProtoConverter(functionCollector); | ||
| Builder builder = io.substrait.plan.Plan.builder(); | ||
| builder.version( | ||
| ImmutableVersion.builder().from(Version.DEFAULT_VERSION).producer("isthmus").build()); | ||
|
|
||
| // TODO: consider case in which one sql passes conversion while others don't | ||
| sqlToRelNode(sql, validator, catalogReader) | ||
| .forEach( | ||
| root -> { | ||
| plan.addRelations( | ||
| PlanRel.newBuilder() | ||
| .setRoot( | ||
| relProtoConverter.toProto( | ||
| SubstraitRelVisitor.convert( | ||
| root, EXTENSION_COLLECTION, featureBoard)))); | ||
| }); | ||
| functionCollector.addExtensionsToPlan(plan); | ||
| return plan.build(); | ||
| sqlToRelNode(sql, validator, catalogReader).stream() | ||
| .map(root -> SubstraitRelVisitor.convert(root, EXTENSION_COLLECTION, featureBoard)) | ||
| .forEach(root -> builder.addRoots(root)); | ||
|
|
||
| PlanProtoConverter planToProto = new PlanProtoConverter(); | ||
|
|
||
| return planToProto.toProto(builder.build()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call on wiring this through the POJOs |
||
| } | ||
|
|
||
| private List<RelRoot> sqlToRelNode( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.