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

feat(isthmus): support for SQL expressions in CLI #209

Merged
merged 44 commits into from
Feb 6, 2024
Merged

feat(isthmus): support for SQL expressions in CLI #209

merged 44 commits into from
Feb 6, 2024

Conversation

davisusanibar
Copy link
Contributor

To close #207

davisusanibar and others added 26 commits October 24, 2023 12:10
@davisusanibar
feat: convert sql expression into proto extended expressions
333989f
@davisusanibar
fix: implement nameToNodeMap and nameToTypeMap dyamically instead of …
f4b6581 
…hard coded
@davisusanibar
fix: cover support also for project extended expression
a79f57d
@davisusanibar
fix: cover support also for project extended expression
a37be92
@davisusanibar
fix: create schema dynamically
9f6aaf3
@davisusanibar
fix: set function reference and extensions dinamically
52b41e3
@davisusanibar
Merge branch 'main' into feature/sql_to_extended_expression
74d13d3
@davisusanibar
fix: clean code
3d80d1f
@davisusanibar
Merge branch 'main' into feature/sql_to_extended_expression
ae84176
@davisusanibar
fix: clean code
5954a62
@davisusanibar
fix: rename variables to clean code
fc33a32
@davisusanibar
fix: from/to pojo/protobuf
217f2a0
@davisusanibar
feat: enable support from/to pojo/protobuf for extended expressions
75e4f48
@davisusanibar
Merge branch 'main' into feature/from_to_protobuf_pojo
1d23187
@davisusanibar
fix: consume core module for proto/pojo conversions
5adc79f
@davisusanibar
fix: clean code redundant method
940f703
@davisusanibar
Merge branch 'main' into feature/sql_to_extended_expression
e281f2f
@davisusanibar @danepitkin
fix: apply suggestions from code review
f817eb0 
Co-authored-by: Dane Pitkin <[email protected]>
@davisusanibar
fix: code review core module
b1c96bd
@davisusanibar
fix: code review core module testing side
3d9b927
@davisusanibar
feat: support aggregation function in extended expression from/to poj…
e790492 
…o/proto
@davisusanibar
fix: merge from/to proto/pojo
ef7c076
@davisusanibar
fix: merge from/to proto/pojo
d1b4efb
@davisusanibar
fix: merge from/to proto/pojo + solve comments on the PR
c26fecd
@davisusanibar
Merge branch 'main' into feature/sql_to_extended_expression
bdde874
@davisusanibar
feat: expose sql expression to extended expression thru Pico CLI
c3cd3e6
@davisusanibar
Copy link
Contributor Author

This PR depend of: #206 / #191

@davisusanibar davisusanibar force-pushed the feature/sql_to_extended_expression_picocli branch from c8f7e62 to 8d0f81d Compare December 12, 2023 21:27
@davisusanibar davisusanibar mentioned this pull request Dec 12, 2023
Open
4 tasks
vibhatha
vibhatha reviewed Feb 3, 2024
View reviewed changes
isthmus/src/main/java/io/substrait/isthmus/IsthmusEntryPoint.java Outdated
@@ -61,20 +68,40 @@ enum OutputFormat {
// this example implements Callable, so parsing, error handling and handling user
// requests for usage help or version help can be done with one line of code.
public static void main(String... args) {
int exitCode = new CommandLine(new PlanEntryPoint()).execute(args);
logger.debug(Arrays.toString(args));
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just deleted log options, and added https://github.com/remkop/picocli/blob/main/picocli-codegen/README.adoc#213-gradle to helps us with --help and --version options.

vibhatha
vibhatha reviewed Feb 3, 2024
View reviewed changes
Copy link
Contributor

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

@davisusanibar added a few comments.

@vibhatha
Copy link
Contributor

vibhatha commented Feb 3, 2024

@davisusanibar
nit: should we use System.out. or logger.info

@davisusanibar
Copy link
Contributor Author

Nice work! Just one more thing:

Can we update the Isthmus README with the new cli arg? https://github.com/substrait-io/substrait-java/blob/main/isthmus/README.md

Just updated, and also added a support for --help and --version options.

@davisusanibar
Copy link
Contributor Author

@davisusanibar nit: should we use System.out. or logger.info

I deleted log options and added logic to handle:

  • If any parameters were passed, a usage message would appear
  • Support for helpers --help
  • Version option support --version

vibhatha
vibhatha approved these changes Feb 4, 2024
View reviewed changes
Copy link
Contributor

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

Thanks @davisusanibar. LGTM!

@vibhatha vibhatha requested a review from vbarua February 4, 2024 04:51
@davisusanibar
Copy link
Contributor Author

A new issue has been created to evaluate the possibility of synchronizing the current fixed value version with the semantic release version: #223

@davisusanibar
Copy link
Contributor Author

@vibhatha, would it be possible for you to review this again?

A change was just made to allow passing lists of expressions and expression sequences by separator (comma by default).

danepitkin
danepitkin approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@danepitkin danepitkin left a comment

Choose a reason for hiding this comment

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

Thanks David! LGTM

vbarua
vbarua requested changes Feb 5, 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.

Left some comments around using string splitting to handle multiple expressions.

isthmus/src/main/java/io/substrait/isthmus/IsthmusEntryPoint.java Outdated Show resolved Hide resolved
isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java Outdated Show resolved Hide resolved
@davisusanibar davisusanibar requested a review from vbarua February 5, 2024 23:37
@vbarua
Copy link
Member

vbarua commented Feb 6, 2024

@davisusanibar I had some small feedback which I decided to turn into a commit. If it looks good to you, I can squash and merge this tomorrow.

908d5e3

@davisusanibar
Copy link
Contributor Author

@davisusanibar I had some small feedback which I decided to turn into a commit. If it looks good to you, I can squash and merge this tomorrow.

908d5e3

Thanks a lot for this kind of refinement all the time. I appreciate your assistance.

vbarua
vbarua approved these changes Feb 6, 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.

Changes look good. Thanks for working on this @davisusanibar!

@vbarua vbarua changed the title feat: expose extended expression on Isthmus graal image using PicoCLI feat(isthmus): support for SQL expressions in CLI Feb 6, 2024
@vbarua vbarua merged commit e63388d into substrait-io:main Feb 6, 2024
8 checks passed
ajegou pushed a commit to ajegou/substrait-java that referenced this pull request Mar 29, 2024
@davisusanibar
feat(isthmus): support for SQL expressions in CLI (substrait-io#209)
80c990d
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

@danepitkin danepitkin danepitkin approved these changes

@vibhatha vibhatha Awaiting requested review from vibhatha

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

Successfully merging this pull request may close these issues.

[Isthmus][CLI] Expose ExtendedExpression on Isthmus graal image using PicoCLI
4 participants
@davisusanibar @vibhatha @vbarua @danepitkin