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

Fix #906 #908

Merged
merged 1 commit into from
Jun 23, 2014
Merged

Fix #906 #908

merged 1 commit into from
Jun 23, 2014

Conversation

johnynek
Copy link
Collaborator

closes #906

@jcoveney
Copy link
Contributor

why can't they just explicitly use Fields.REPLACE as the toField?

@johnynek
Copy link
Collaborator Author

@jcoveney that is not exactly what that syntax means. (inputs -> outputs) means map these inputs onto these outputs. So, the question is what does it mean when the results are called ALL? I guess it means we discard the inputs and keep the outputs (which are the new ALL).

That said, he could also have used mapTo to get the same result (which always uses RESULTS).

But, his point is that cascading throws an error in this case now, so it fails at runtime when in could arguably have a meaning. I'm fine with adding or not adding this code. I think everyone should use the TypedAPI anyway.

We should really think about macros or shapeless integration that could get the best of both type-safety and ad-hoc records.

@jcoveney
Copy link
Contributor

Agreed on the macro use.

That said, what would be expected in the case where we have fields 'one and 'two, and then

pipe.mapTo(Fields.ALL->Fields.ALL) { t: Tuple => t.add("hey") }

With this patch, would the new fields be 'one, 'two, then an unnamed "2" field? I guess it's just dangerous to go to ALL because then the naming is unspecified, especially if the same thing can be done with a mapTo...

@ianoc
Copy link
Collaborator

ianoc commented Jun 23, 2014

In cascading there are lots of opportunities to loose the names in the tuples or just not name them, i'm not sure we should be attempting to not add features from it. If the user is calling a t.add without specifying a field name... well i guess they should know what they are doing or expect the names to be gone. I'd rather Fields.ALL -> Fields.ALL has the meaning you would expect(replace).

@johnynek
Copy link
Collaborator Author

I agree this is dangerous because it may be easy to get the field order wrong later, but I also agree with Ian that it is better that this have some meaning than to fail at plan-time.

@johnynek
Copy link
Collaborator Author

So merge?

ianoc added a commit that referenced this pull request Jun 23, 2014
@ianoc ianoc merged commit 3d4298f into develop Jun 23, 2014
@ianoc ianoc deleted the alltoallfix branch June 23, 2014 20:21
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.

FieldConversions#defaultMode doesn't handle fromFields=ALL, toFields=ALL
3 participants