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.

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

Provide PlanBuilder convenience function #1578

Closed
wants to merge 1 commit into from

Conversation

pedroerp
Copy link
Contributor

Summary:
Add a PlanBuilder.tableScane helper function to reduce the amount of
boilerplace when pulling data out of TPC-H connector.

Differential Revision: D36303299

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels May 11, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D36303299

Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks for this change. Makes the code very clean.

/// @param columnNames The columns to be returned from that table.
PlanBuilder& tableScan(
const std::shared_ptr<connector::tpch::TpchTableHandle>& tpchTableHandle,
std::vector<std::string>&& columnNames);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the "columnNames" parameter be const && ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean const &? The intent here is to move it in to this this function since the ROW() function expects an rvalue ref (so we can avoid useless copies)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright. Thanks.

@@ -321,6 +321,10 @@ RowTypePtr getTableSchema(Table table) {
return nullptr; // make gcc happy.
}

TypePtr resolveTpchColumn(Table table, const std::string& columnName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for not making "table" parameter const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Table is a enum (a single byte), so const ref is more expensive than passing by value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable. Thanks.

Summary:
Pull Request resolved: facebookincubator#1578

Add a PlanBuilder.tableScane helper function to reduce the amount of
boilerplace when pulling data out of TPC-H connector.

Reviewed By: kagamiori

Differential Revision: D36303299

fbshipit-source-id: 854e1477092fca8b342b1c9971abfb7eeeb8a578
@pedroerp pedroerp force-pushed the export-D36303299 branch from 80a4e00 to 84434e0 Compare May 11, 2022 17:00
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D36303299

Arty-Maly pushed a commit to Arty-Maly/velox that referenced this pull request May 13, 2022
Summary:
Pull Request resolved: facebookincubator#1578

Add a PlanBuilder.tableScane helper function to reduce the amount of
boilerplace when pulling data out of TPC-H connector.

Reviewed By: kagamiori

Differential Revision: D36303299

fbshipit-source-id: d5898559a94c97850a38711a90202caf24e8fca6
zhejiangxiaomai pushed a commit to zhejiangxiaomai/velox that referenced this pull request Jun 21, 2022
Summary:
Pull Request resolved: facebookincubator#1578

Add a PlanBuilder.tableScane helper function to reduce the amount of
boilerplace when pulling data out of TPC-H connector.

Reviewed By: kagamiori

Differential Revision: D36303299

fbshipit-source-id: d5898559a94c97850a38711a90202caf24e8fca6
shiyu-bytedance pushed a commit to shiyu-bytedance/velox-1 that referenced this pull request Aug 18, 2022
Summary:
Pull Request resolved: facebookincubator#1578

Add a PlanBuilder.tableScane helper function to reduce the amount of
boilerplace when pulling data out of TPC-H connector.

Reviewed By: kagamiori

Differential Revision: D36303299

fbshipit-source-id: d5898559a94c97850a38711a90202caf24e8fca6
marin-ma pushed a commit to marin-ma/velox-oap that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants