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

Merge dataframe and dataframe imp #1998

Merged
merged 10 commits into from
Mar 15, 2022
Merged

Merge dataframe and dataframe imp #1998

merged 10 commits into from
Mar 15, 2022

Conversation

vchag
Copy link
Contributor

@vchag vchag commented Mar 12, 2022 •
edited
Loading

Which issue does this PR close?

Closes #1962 .

Rationale for this change

The Dataframe trait was introduced in order to have two separate Dataframe implementation in Datafusion and Ballista. Since then, the design has changed and we are sharing the same dataframe implementation between Datafusion and Ballista now. Therefore, we could simplify the code base by getting rid of the Dataframe trait abstraction and promoting DataframeImpl into Dataframe struct.

What changes are included in this PR?

  • Merging Dataframe trait and DataframeImpl into a single Dataframe struct
  • Deleting datafusion/src/execution/dataframe_impl.rs

venkata.chaganti added 2 commits March 12, 2022 13:56
merging Dataframe trait and DataFrameImpl into Dataframe struct
7e4f4e9 
Change-Id: Id15520a81c272a7666f17d5f3b707445a4af7798
reformatting
f3e1139 
Change-Id: I6c8cab772c206c80cd5aa5b39296d8009ab4efdb
@github-actions github-actions bot added ballista datafusion Changes in the datafusion crate labels Mar 12, 2022
venkata.chaganti added 3 commits March 12, 2022 14:26
new DataFrame type fixes
3e65ef7 
Change-Id: I01ebf2f52e21aaf9b24e4fb7ac54904dcd55e220
fix build
90b3d75 
Change-Id: Iefb549ecee1648a89e1165f93a8e2fa5e785df56
re-formatting files
92d6fdf 
Change-Id: I2c96a7e24f1f74052eee50ea8199f381bdb553df
xudong963
xudong963 reviewed Mar 13, 2022
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @vchag

Btw, some examples in datafusion-examples/examples still use dataframe impl.

@houqp houqp added the api change Changes the API exposed to users of the crate label Mar 13, 2022
@vchag
Copy link
Contributor Author

vchag commented Mar 14, 2022

LGTM, thank you @vchag

Btw, some examples in datafusion-examples/examples still use dataframe impl.

You're welcome @xudong963
Thanks for pointing it out.

The log mentions of an
unresolved import datafusion::execution::dataframe_impl
in datafusion-examples/examples/custom_datasource.rs:24:28

datafusion-examples/examples/custom_datasource.rs isn't present in
datafusion-examples.

Could the build process be using an old version of datafusion-examples module?

revise dataframe.rs
47af10a 
Change-Id: I3aea55073f454d9fd096df0d5d9bf7fe5533bfaa
@xudong963
Copy link
Member

xudong963 commented Mar 14, 2022 •
edited
Loading

datafusion-examples/examples/custom_datasource.rs isn't present in
datafusion-examples.

It exists in master
image

However, the ticket didn't delete it
image

strange...

vchag and others added 4 commits March 14, 2022 14:03
@vchag
Merge branch 'apache:master' into merge_dataframe_and_dataframeImp
78dddc4
fix import statements
fae8b34 
Change-Id: I72f7bf086cf8a5726490b9cdf3bd52ff4154f006
re-formatting files
8a818bc 
Change-Id: Ib585edb0b6c7416c48a77168d82989e309dd776b
bug fix
21621b4 
Change-Id: I1af2eb7609912074b37e38f66c15d0ce75ed291a
@vchag
Copy link
Contributor Author

vchag commented Mar 14, 2022

datafusion-examples/examples/custom_datasource.rs isn't present in
datafusion-examples.

It exists in master image

However, the ticket didn't delete it image

strange...

Sorry, my bad. My branch was lagging. Merging apache: master into my branch fixed the issue.

xudong963
xudong963 approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks again @vchag

cc @houqp

yjshen
yjshen approved these changes Mar 15, 2022
View reviewed changes
Copy link
Member

@yjshen yjshen left a comment

Choose a reason for hiding this comment

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

Thanks @vchag! Very neat refactoring!

@yjshen yjshen merged commit 7158b4b into apache:master Mar 15, 2022
@yjshen
Copy link
Member

yjshen commented Mar 15, 2022

Thanks @vchag @xudong963 !

@vchag
Copy link
Contributor Author

vchag commented Mar 15, 2022

Thanks @vchag @xudong963 !

yw!

@houqp
Copy link
Member

houqp commented Mar 17, 2022

nice work @vchag !

@houqp houqp mentioned this pull request Mar 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yjshen yjshen yjshen approved these changes

@xudong963 xudong963 xudong963 approved these changes

Assignees
No one assigned
Labels
api change Changes the API exposed to users of the crate datafusion Changes in the datafusion crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge Dataframe trait and DataframeImpl into Dataframe struct
4 participants
@vchag @xudong963 @yjshen @houqp