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

Variant: Remove JSON conversion table #485

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

rdblue
Copy link
Contributor

@rdblue rdblue commented Feb 21, 2025

Rationale for this change

I've talked with several community members that intend to implement JSON conversion, but do not want the conversion specified in the Variant spec to be required. If the conversion is not required, I don't think that it makes sense to add it to the spec.

The conversion was originally added because we wanted to ensure that engines that do not support Variant could interact with the data. At the time, it seemed like a good idea to standardize how conversion to JSON took place for those engines. But there is necessarily data loss when converting to JSON. It's hard to decide just how lossy this conversion (to JSON and back) should be and what value there is to minimizing the loss. That's why people don't intend to implement a consistent well-defined conversion. Without willingness to follow this section of the spec, it should be removed.

What changes are included in this PR?

Remove the JSON conversion table and section from the Variant spec.

Do these changes have PoC implementations?

No. The conversions in this table were never implemented.

Copy link
Contributor

@emkornfield emkornfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still believe the best course of action is to move this to an appendix and make it clear that is optional. Otherwise there are cases where engines might diverge needlessly (e.g. special values for double, and bytes encoding). But I'm OK removing it.

@rdblue
Copy link
Contributor Author

rdblue commented Feb 21, 2025

I still believe the best course of action is to move this to an appendix and make it clear that is optional. Otherwise there are cases where engines might diverge needlessly (e.g. special values for double, and bytes encoding). But I'm OK removing it.

Does it matter if behavior differs? If it does, then I'm fine keeping this as required so that it doesn't diverge. But if this is optional, it doesn't help prevent divergence.

@emkornfield
Copy link
Contributor

Does it matter if behavior differs? If it does, then I'm fine keeping this as required so that it doesn't diverge. But if this is optional, it doesn't help prevent divergence.

I don't think the statement is a binary here. In practice it probably doesn't matter, but it still doesn't mean that we wouldn't necessarily want consistency here. It becomes an implementation details, but in the absence of a common guide different implementations might choose slightly different representations, not for any good technical reason but simply because it is arbitrary (i.e. "Inf" vs "Infinity"). This is why I think an appendix on implementation notes, that indicate this is one possible way of translating data and is recommended but not required makes sense to me. This is not part of the spec, but that doesn't mean that providing guidance is not beneficial.

@rdblue
Copy link
Contributor Author

rdblue commented Feb 24, 2025

This is not part of the spec, but that doesn't mean that providing guidance is not beneficial.

But what is the value if that guidance is not reliable? If you're going to choose an arbitrary thing like "-Inf", then it needs to be done that way everywhere. You can't also allow "Inf (but _not_ the positive one)" because people can't rely on the recommendation. If it is something for compatibility, it should to be required. Otherwise it doesn't add compatibility.

@rdblue
Copy link
Contributor Author

rdblue commented Feb 24, 2025

I talked with Micah on Slack. We agreed that we can have a larger discussion on the mailing list, but can merge this now. Since this is probably not going to be a hard requirement, I think we should remove it and add it back later rather than leave it in until the discussion concludes.

@rdblue rdblue closed this Feb 24, 2025
@rdblue rdblue reopened this Feb 24, 2025
@rdblue rdblue merged commit 855a58c into apache:master Feb 24, 2025
8 checks passed
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.

4 participants