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

[FLINK-3993] [py] Add generateSequence() support to Python API #2055

Closed
wants to merge 3 commits into from

Conversation

omaralvarez
Copy link
Contributor

@omaralvarez omaralvarez commented May 31, 2016

Addition of generate_sequence() to python API:

data = env.generate_sequence(1,20)

If a parallel number source was needed, there was no option to create it, since from_elements() is not parallel.

@zentol
Copy link
Contributor

zentol commented May 31, 2016

Thank you for your contribution. Overall the code is correct, but i would prefer if you would the various "frm" fields to "from".

Also, a small update to the documentation, and a simple test would be appreciated.

@omaralvarez
Copy link
Contributor Author

omaralvarez commented May 31, 2016

About the "frm", in Python when using from, there is a syntax error since from is a system reserved word. In the Java side of things there is no problem with renaming and using from, but I chose to use also frm to maintain consistency.

I will get right on the documentation and test. Should I add commits or squash them in the one commit?

@zentol
Copy link
Contributor

zentol commented May 31, 2016

please add them as a separate commit, they will be squashed when merging.

Good point regarding frm.

@omaralvarez
Copy link
Contributor Author

I think, everything should be ready now. Although while performing testing, I have found something that I think is not ideal.

If we use this code:

env.generate_sequence(1, 5)\
         .map(Id()).map_partition(Verify([1,2,3,4], "Sequence")).output()

The Verify() function will error out with IndexError: list index out of range, this is not ideal, since it should raise a Flink testing exception. I could also try to fix this if needed.

@zentol
Copy link
Contributor

zentol commented Jun 1, 2016

Changing the test infrastructure should be done as a separate issue. There are a few things that could be improved, especially a bit of documentation (like what is the difference between Verify and Verify2).

Your changes look good, I'll merge them later today.

@omaralvarez
Copy link
Contributor Author

Perfect. I'll open a new issue and try to fix that and document the test scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants