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

[#764] added ChangeLabel and ChangePropertyKeys UDFs #772

Merged
merged 4 commits into from
May 8, 2018
Merged

[#764] added ChangeLabel and ChangePropertyKeys UDFs #772

merged 4 commits into from
May 8, 2018

Conversation

merando
Copy link
Contributor

@merando merando commented Apr 27, 2018

two simple UDFs for changing a specified label and another one for changing property keys.

Copy link
Contributor

@smee smee left a comment

Choose a reason for hiding this comment

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

It would be nice to have at least one unit test per transformation function, for usage/documentation purposes as well as for the test coverage.

@Override
public EPGMElement apply(EPGMElement current, EPGMElement transformed) {

for (Map.Entry<String, String> mapping : keyMappings.entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TransformBase does not set the properties in the EPGMElement transformed, which means, that this transformation will only set the changed properties, everything else will be lost. We need to copy all non-transformed properties as well or transform and return current instead of transformed.

public EPGMElement apply(EPGMElement current, EPGMElement transformed) {

if (current.getLabel().equals(oldLabel)) {
transformed.setLabel(newLabel);
Copy link
Contributor

Choose a reason for hiding this comment

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

transformed does not contain any properties. I suggest to change and return current instead.

@merando
Copy link
Contributor Author

merando commented May 2, 2018

Oh ok, thanks for the advice. I got the comment stating its a "copy" wrong.

@merando
Copy link
Contributor Author

merando commented May 2, 2018

Again an unstable foodbroker test?

@smee
Copy link
Contributor

smee commented May 2, 2018

@merando Yeah, this test seems inherently instable. There is a ticket #769. I think we should do the suggested extraction of the generators from issue #617 so that these errors don't show up on changes to gradoop-flink.

@s1ck
Copy link
Member

s1ck commented May 2, 2018

I requested some support by @p3et for #769 I'll reach out again.

@s1ck
Copy link
Member

s1ck commented May 3, 2018

I just merged the PR to fix the flaky test. Try a rebuild pls

@merando
Copy link
Contributor Author

merando commented May 3, 2018

Seems to work now! Great patch, thanks @s1ck & @p3et

@smee
Copy link
Contributor

smee commented May 7, 2018

@merando told me, that the UDFs in this PR don't yet work as intended. Could you please remove this "NOT READY FOR MERGE" label as soon as you are satisfied with the functionality?

@merando
Copy link
Contributor Author

merando commented May 8, 2018

ok, it works now. I just forgot to add type information

@smee
Copy link
Contributor

smee commented May 8, 2018

What do you think about renaming the two functions to RenameLabel and RenamePropertyKeys? It describes the intention and leaves the current names for potential future implementations that change the label/properties in more complex ways than renaming.

@merando
Copy link
Contributor Author

merando commented May 8, 2018

Sounds good to me. I'll provide the commit right away

@smee smee merged commit 75bc6ab into dbs-leipzig:master May 8, 2018
0x002A pushed a commit to ChrizZz110/gradoop that referenced this pull request Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants