Skip to content

Comments

Initial commit of substrait-cpp#2

Merged
westonpace merged 15 commits intosubstrait-io:mainfrom
chaojun-zhang:main_backup
Dec 11, 2022
Merged

Initial commit of substrait-cpp#2
westonpace merged 15 commits intosubstrait-io:mainfrom
chaojun-zhang:main_backup

Conversation

@chaojun-zhang
Copy link
Contributor

@chaojun-zhang chaojun-zhang commented Oct 29, 2022 •
edited
Loading

This PR is an Initial commit of substrait-cpp which includes following features:

  • parse substrait extension yaml file
  • function and type support
  • functin implementation lookup by a function signature

@chaojun-zhang
Copy link
Contributor Author

chaojun-zhang commented Oct 29, 2022 •
edited
Loading

Due to can't re-open PR for #1, so create this new PR

westonpace
westonpace requested changes Nov 8, 2022
View reviewed changes
@westonpace
Copy link
Member

westonpace commented Nov 8, 2022

Thanks for addressing my earlier comments. I made another pass today and appreciate all the work! 😃

chaojun-zhang and others added 10 commits November 8, 2022 13:50
@chaojun-zhang @westonpace
Update README.md
a3cce71 
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@chaojun-zhang @westonpace
Update README.md
cc3bf4f 
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@chaojun-zhang @westonpace
Update include/substrait/common/Exceptions.h
f5e9d6f 
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@chaojun-zhang @westonpace
Update substrait/common/Exceptions.cpp
ed29e44 
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@chaojun-zhang @westonpace
Update include/substrait/function/FunctionLookup.h
a78a513 
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@chaojun-zhang @westonpace
Update include/substrait/function/Extension.h
ae7299c 
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@chaojun-zhang
fix issues
db6a202
@chaojun-zhang
fix issues
2568fc1
@chaojun-zhang
rename functionVariant to functionImplementation
70c7a31
@chaojun-zhang
use spdx license header
3c5acae
@chaojun-zhang
Copy link
Contributor Author

chaojun-zhang commented Nov 8, 2022 •
edited
Loading

Thanks for addressing my earlier comments. I made another pass today and appreciate all the work! 😃

@westonpace, Thank you for your patience to reviewing again, and I have updated this PR according your reviews.

westonpace
westonpace approved these changes Dec 11, 2022
View reviewed changes
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review. I have one more nitpick about variant vs. implementation terminology but we can address that in a follow-up. This is getting rather large to continue reviewing and it is close enough I want to merge now.

include/substrait/function/Extension.h
Comment on lines +44 to +50
void addScalarFunctionVariant(const FunctionImplementationPtr& functionVariant);

/// Add a aggregate function variant.
void addAggregateFunctionVariant(const FunctionImplementationPtr& functionVariant);

/// Add a window function variant.
void addWindowFunctionVariant(const FunctionImplementationPtr& functionVariant);
Copy link
Member

@westonpace westonpace Dec 11, 2022

Choose a reason for hiding this comment

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

We should probably rename all these addXyzVariant to addXyzImplementation.

@westonpace westonpace merged commit 041fecb into substrait-io:main Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpace westonpace westonpace approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chaojun-zhang @westonpace