这是indexloc提供的服务，不要输入任何密码
Skip to content

Fix aggregateByKey python binding #16

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
Closed
wants to merge 1 commit into from
Closed

Fix aggregateByKey python binding #16

wants to merge 1 commit into from

Conversation

@rahulyesantharao
Copy link
Collaborator

@rahulyesantharao rahulyesantharao commented Jul 26, 2021

The python binding for aggregateByKey (in dataset.py) currently crashes due to the deprecated function get_lambda_source - update to the new function get_udf_source.

@ms705 ms705 requested a review from LeonhardFS July 27, 2021 14:36
ms705
ms705 approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@ms705 ms705 left a comment

Choose a reason for hiding this comment

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

LGTM; let's merge if @LeonhardFS approves.

@LeonhardFS
Copy link
Contributor

LeonhardFS commented Jul 29, 2021

Great, to get this fixed - yet I feel this PR could benefit from adding a (python) test targeting specifically aggregateByKey to the code base, so we will be alerted in the future and won't forget the code accordingly.

Also, is get_lambda_source still a valid function? If it's deprecated, we probably should remove it.

LeonhardFS
LeonhardFS requested changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@LeonhardFS LeonhardFS left a comment

Choose a reason for hiding this comment

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

pls add a python test for aggregateByKey & check whether get_lambda-source still exists somewhere in the code base.

@LeonhardFS
Copy link
Contributor

LeonhardFS commented Jul 29, 2021

Another thing to maybe improve here is to not use a catch-all try:.... except:... block which basically lead to silent failure, but rather test for a specific CodeExtraction Exception.

@rahulyesantharao
Copy link
Collaborator Author

rahulyesantharao commented Jul 30, 2021 •
edited
Loading

get_lambda_source is defined in interactive_shell.py and is used in get_source in reflection.py - in other words, it is called by the function get_udf_source that replaced it in this file.

@LeonhardFS LeonhardFS linked an issue Aug 4, 2021 that may be closed by this pull request
[BUG] AggregateByKey uses old, deprecated function for source code extraction/reflection #18
Closed
@LeonhardFS
Copy link
Contributor

LeonhardFS commented Aug 4, 2021

superseded by #19 , closed for now.

@LeonhardFS LeonhardFS closed this Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LeonhardFS LeonhardFS LeonhardFS requested changes

@ms705 ms705 ms705 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.

[BUG] AggregateByKey uses old, deprecated function for source code extraction/reflection

3 participants

@rahulyesantharao @LeonhardFS @ms705