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.

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

Jump to bottom

docs(spark): add substrait-spark usage examples #293

Merged
merged 8 commits into from
Oct 4, 2024
Merged

docs(spark): add substrait-spark usage examples #293

merged 8 commits into from
Oct 4, 2024

Conversation

mbwhite
Copy link
Contributor

@mbwhite mbwhite commented Sep 9, 2024 •
edited
Loading

This adds a new Substrait-Spark example to the repo

  • example code on creating and consuming substrait plans
  • details in the example of how the plans are converted
  • place to add further examples
  • add to build process

Note I've not added running up the Spark engine to test - as I don't to blow any of the github actions time or capacity.

@mbwhite mbwhite mentioned this pull request Sep 9, 2024
Closed
@mbwhite mbwhite force-pushed the substrait-spark-example branch 8 times, most recently from 3d8df4e to d7ea257 Compare September 9, 2024 14:37
@mbwhite mbwhite marked this pull request as ready for review September 16, 2024 08:15
vbarua
vbarua reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

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

Was able to make some time to look at part of this. Left some questions.

examples/substrait-spark/README.md Outdated Show resolved Hide resolved
examples/substrait-spark/README.md Outdated Show resolved Hide resolved
examples/substrait-spark/app/build.gradle Outdated Show resolved Hide resolved
examples/substrait-spark/app/build.gradle Outdated Show resolved Hide resolved
examples/substrait-spark/app/src/main/java/io/substrait/examples/SparkDataset.java Outdated Show resolved Hide resolved
examples/substrait-spark/gradlew Outdated Show resolved Hide resolved
@vbarua
Copy link
Member

vbarua commented Sep 23, 2024

@mbwhite I'm also open to having this be its own build if that makes more sense. I just noticed it wasn't integrated into the main build and was curious how difficult it would be to do so. There are some advantages to keeping it separate.

@mbwhite mbwhite force-pushed the substrait-spark-example branch 3 times, most recently from 8e26144 to 47c4a53 Compare September 23, 2024 15:27
@mbwhite
Copy link
Contributor Author

mbwhite commented Sep 23, 2024

@vbarua I think it's a bit simpler now which is good, and the editor does pick it up properly. But still should be clear enough to for anybody to use to make their own application.

Also moved to Kotlin; hopefully got it right!

(though admit gradle did put up more resistence than I expected :-) )

@mbwhite mbwhite requested a review from vbarua September 24, 2024 08:06
vbarua
vbarua reviewed Oct 1, 2024
View reviewed changes
Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

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

I've gone ahead and configured and run spotless, and also applies some minor doc changes. Let me know if those seem reasonable to you. I've also left some questions inline.

There's parts of the README that seem to reference functionality that's not in this PR. Did these examples come from some other repo?

examples/substrait-spark/src/main/java/io/substrait/examples/SparkHelper.java Outdated Show resolved Hide resolved
examples/substrait-spark/src/main/java/io/substrait/examples/SparkHelper.java Outdated Show resolved Hide resolved
examples/substrait-spark/src/main/java/io/substrait/examples/SparkHelper.java Outdated Show resolved Hide resolved
examples/substrait-spark/README.md Outdated Show resolved Hide resolved
examples/substrait-spark/README.md Outdated Show resolved Hide resolved
examples/substrait-spark/README.md Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Oct 1, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@vbarua vbarua self-requested a review October 3, 2024 00:14
@vbarua
Copy link
Member

vbarua commented Oct 3, 2024

Rebased of off main to clean up the diff.

@vbarua vbarua mentioned this pull request Oct 3, 2024
Open
vbarua
vbarua reviewed Oct 3, 2024
View reviewed changes
Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

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

Made some minor doc changes in 1f59203

@mbwhite Overall I think this is close. There's one example I've commented on which I still don't quite understand. I'm inclined to remove it and revisit it later if that works for you.

examples/substrait-spark/src/main/java/io/substrait/examples/util/ExpressionStringify.java

/** ExpressionStringify gives a simple debug text output for Expressions */
public class ExpressionStringify extends ParentStringify
implements ExpressionVisitor<String, RuntimeException> {
Copy link
Member

Choose a reason for hiding this comment

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

meta: this doesn't seem to fully implement a visitation for all expressions. Doing so it out of scope for this, but I've added a comment indicating that this is still a work in progress and linked it to #302

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me :-)

examples/substrait-spark/src/main/java/io/substrait/examples/util/SubstraitStringify.java Show resolved Hide resolved
examples/substrait-spark/README.md Outdated Show resolved Hide resolved
mbwhite and others added 6 commits October 4, 2024 08:34
@mbwhite
feat: review Comments
0ef64e8 
Address the review comments

Also adjusted the build so it was connected to the main top level build
(resulted in removing quite a bit of the gradle build that really wasn't needed here)

Converted to Kotlin format (hopefully corectly)

Signed-off-by: MBWhite <[email protected]>
@vbarua @mbwhite
style(spark): configure and apply spotless
e6018ab
@vbarua @mbwhite
docs: minor type fixes
c115600
@vbarua @mbwhite
build: bump spotless plugin version
4fce4e7
@mbwhite
feat: addressed comments and added missing code
e71ebab 
Signed-off-by: MBWhite <[email protected]>
@vbarua @mbwhite
docs: minor doc updates
dc89c80
@vbarua vbarua changed the title feat: spark-substrait example docs(spark): add substrait-spark usage examples Oct 4, 2024
@vbarua vbarua merged commit 91ce863 into substrait-io:main Oct 4, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbarua vbarua vbarua approved these changes

@EpsilonPrime EpsilonPrime Awaiting requested review from EpsilonPrime

@zeroshade zeroshade Awaiting requested review from zeroshade

@richtia richtia Awaiting requested review from richtia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mbwhite @vbarua @CLAassistant