-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[SPARK-18122][SQL][WIP]Fallback to Kryo for unsupported encoder for class's subfield #15918
Conversation
Test build #68764 has finished for PR 15918 at commit
|
I think this introduces a big behavior change, right? now objects are serialized with a hybrid of two serializers? I am not sure this is a good idea. |
Yes, and there is a aspect to be concerned: |
There's not obviously a way to implement this, IMHO. I am not sure mixing two serializations would make sense. I would close these for now. |
FWIW I think there are some value here, but I agree that changing the default behavior can be surprising and bad. |
@srowen and @rxin what is the default behavior that is changed here? i see a current situation where an implicit encoder is provided that simply cannot handle the task at hand and this leads to failure. either the implicits for ExpressionEncoder need to be more narrow so that they do not claim types they cannot handle (and then other implicit encoders can be used), or they need to be able to handle these types, for example by falling back to kryo as is suggested in this JIRA. currrently i proposed making the ExpressionEncoders more narrow (that seemed the easier fix to me at first) but @marmbrus preferred the approach of falling back to kryo and broadening it. see: |
I agree with @koertkuipers that the only change in behavior is that cases that used to throw an error will now not throw an error. If done right (I haven't looked deeply at the PR itself yet), no case that is currently working should change. It is maybe slightly odd to mix serialization types, but thats kind of already happening today if you use the The one possible concern compatibility concern I can see is, if in the future we add support for an previously unsupported type, the schema will change from Thoughts? |
We should probably add a flag (maybe even off by default). The error message can tell you to turn on the flag if you are okay with the fallback. |
if we do a flag i would also prefer it if the current implicits are more narrow if the flag is not set, if possible. |
I don't think you can limit the implicit. What type would pick up case classes, but not case classes that contain invalid things? I think you would need a macros for this kind of introspection. (I'd be happy to be proven wrong with a PR.) I'd recommend you only import the implicits you need rather than using the wildcard. |
It can be done with shapeless (which perhaps uses macros under hood, I
don't know).
On Dec 1, 2016 19:56, "Michael Armbrust" <notifications@github.com> wrote:
I don't think you can limit the implicit. What type would pick up case
classes, but not case classes that contain invalid things? I think you
would need a macros for this kind of introspection. (I'd be happy to be
proven wrong with a PR.)
I'd recommend you only import the implicits you need rather than using the
wildcard.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15918 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAyIJLwL-MWdzQGb6Ioe2fr_GgP0rP05ks5rD2zNgaJpZM4K1D7U>
.
|
@windpiger, were you still working on this? I think it would be a useful feature if we can get the tests to pass. |
oh sorry,Recently I was working on other works, I will continue to work on the this soon,and finish it. |
retest this please |
Test build #72873 has finished for PR 15918 at commit
|
Test build #72922 has started for PR 15918 at commit |
Test build #72921 has finished for PR 15918 at commit
|
Test build #72924 has started for PR 15918 at commit |
retest this please |
Test build #72927 has finished for PR 15918 at commit
|
Test build #72936 has finished for PR 15918 at commit
|
@windpiger I see you flipped it back to |
this change of even if I use |
Hi @windpiger, is this still WIP? |
What changes were proposed in this pull request?
It will throw a UnsupportedOperationException when a class's subfield has not supported type Encoder.
This PR will fallback to KryoSer/KryoDeser for this kind of subfields.
before fix
after
How was this patch tested?
unittest added