Skip to content

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Jul 14, 2015

I added the creation of the factory method to the TupleGenerator.

I also fixed some inconsistencies between the generator and generated files: it seems that some modifications were made to generated files without modifying the generator. I incorporated these changes to the generator (see the first three hunks in the generator) (except for some whitespace changes in the first three modified files).

I randomly changed some constructor calls into using the new method for testing and demonstration purposes (see the last two changed files).

@aljoscha
Copy link
Contributor

Should we call it of instead of create? In other parts, such as the windowing helpers we also have of for these shortcut constructors.

@mbalassi
Copy link
Contributor

I also prefer of as a method name.

@mxm
Copy link
Contributor

mxm commented Jul 15, 2015

Thanks for adding the generator methods. That's definitely more convenient than using new. I also prefer of instead of create.

@ggevay
Copy link
Contributor Author

ggevay commented Jul 15, 2015

Ok, of looks better indeed. I will modify it.

@fhueske
Copy link
Contributor

fhueske commented Jul 15, 2015

PR looks good to me except for the purely reformatting changes which should be excluded from the PR.
After that it's good to merge, IMO.
Thanks!

@ggevay
Copy link
Contributor Author

ggevay commented Jul 15, 2015

@fhueske:
I included the reformatting changes because the generator is making them. This is happening because some modifications were made directly to some generated codes in the past, which made them inconsistent with the generator. But I guess the goal should be that the generator program would have no effect on the master. One way to achieve this is to commit the changes the generator makes, and the other way would be to change the generator to follow the reformatting. Do you think that I should change the PR to the latter with respect to the reformattings as well? (I already did the latter with respect to some real changes, see my opening comment.)

@fhueske
Copy link
Contributor

fhueske commented Jul 15, 2015

Ah OK. It's fine to commit the changes if they are caused by the TupleGenerator.
I don't think it's necessary to adapt the generator to avoid the changes.

@fhueske
Copy link
Contributor

fhueske commented Jul 17, 2015

I'm going to merge this PR

@asfgit asfgit closed this in 2d191ab Jul 17, 2015
nikste pushed a commit to nikste/flink that referenced this pull request Sep 29, 2015
nltran pushed a commit to nltran/flink that referenced this pull request Jan 8, 2016
pnowojski pushed a commit that referenced this pull request Jul 3, 2024
Clean up dependencies by centralizing most of the ML function code in it's own module. Also organizes files into a package structure.

Functions, Runtimes, and MLFunctionsModule were left where they were for now.

The below list of Classes/References did NOT move packages as they are imported in metastore/sql-service, so this change should be backwards/forwards compatible from the standpoint of downstream dependencies:
MLModelCommonConstants
MLModelCommonConstants.ModelKind
MLModelCommonConstants.ModelTask
RemoteModelOptions.EncryptionStrategy
RemoteModelValidator
MLEvaluateFunction
MLPredictFunction
MLModelRuntime

Follow-on work: 1) Get MLModelRuntime.validateSchemas to be callable from RemoteModelValidator. 2) move everything referenced by downstream dependencies to a single place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants