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

Add Option[T] support for primitives + string in case class macros #1133

Closed
johnynek opened this issue Dec 16, 2014 · 7 comments
Closed

Add Option[T] support for primitives + string in case class macros #1133

johnynek opened this issue Dec 16, 2014 · 7 comments
Assignees

Comments

@johnynek
Copy link
Collaborator

In #1131 we could use null to represent None.

This is trickier for Options of case classes. On the Setter side, that is no problem. On the Converter side it is not clear what to do if you get some nulls and some non-nulls. You could:

  1. If any of an Optional case class is null, the whole thing is null (might lead to silent data loss).
  2. Throw an exception at runtime.

I can actually see the merit in going with 2. Cascading tuples are already not typesafe, and they can throw exceptions at runtime. So, allowing more expressivity which in the usual operation cannot fail might be a good idea.

@johnynek
Copy link
Collaborator Author

If we add Option[T], we need to make the Fields macro return AnyRef for the type and do all the parsing of it ourselves. The issue is that if we say it is a T, if T is a Number, Cascading will treat null like 0, so by the time we see our Tuple, the information is lost (see LongCoerce.java) which is used in cascading when Fields have a Long type attached.

@isnotinvain
Copy link
Contributor

How would you get to the situation of mixed nulls / non-nulls?

@johnynek
Copy link
Collaborator Author

suppose you defined a schema with case classes, but something else writes it (pig, export from some other system).

I think this should just be an error and the fact is the scheme is wrong here. It is possible we would allow a mode where we a loose and log to warning when we see a nonsensical output.

@johnynek
Copy link
Collaborator Author

Also, when I said do all the parsing ourselves, it is as easy as this:

if (tup.getObject(idx) == null) None
else tup.getInt(idx) // cascading parses strings here, dubious idea, but true

@isnotinvain
Copy link
Contributor

I see. Yeah I'd go with throw a runtime exception if the expectation about the schema is not met

@ianoc ianoc self-assigned this Dec 26, 2014
@johnynek
Copy link
Collaborator Author

@ianoc this is closed now, right?

@ianoc
Copy link
Collaborator

ianoc commented Mar 28, 2015

Yep, i believe so.

@ianoc ianoc closed this as completed Mar 28, 2015
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

No branches or pull requests

3 participants