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

Exclude some types (e.g. floating point types) from Smithy's sets #1072

Closed
david-perez opened this issue Feb 1, 2022 · 5 comments
Closed

Comments

@david-perez
Copy link
Contributor

The docs for the Set shape should be updated to exclude any member types that don't have an obvious reasonable notion of equality as an equivalence relation, or it should otherwise explicitly specify such a notion.

Floating point types (float, double) are at least one category, since IEEE 754 floating point numbers equality only forms a partial equivalence relation.

The following minimal model builds fine:

$version: "1.0"

namespace com.amazonaws.simple

use aws.protocols#restJson1

@restJson1
@title("SimpleService")
service SimpleService {
    version: "2022-01-01",
    operations: [
        Healthcheck,
    ],
}

@http(uri: "/healthcheck?fooKey=bar", method: "GET")
operation Healthcheck {
    input: HealthcheckInputRequest,
    output: HealthcheckOutputResponse
}

structure HealthcheckInputRequest {
    member: FloatSet
}

structure HealthcheckOutputResponse {
}

set FloatSet {
    member: Float
}
@adamthom-amzn
Copy link
Contributor

I'm not sure about this - I've been reviewing IEEE 754-2019 and I don't see anything that definitive about equivalence of finite numbers. Granted, it's a large spec and I've been scanning it, so maybe there's something here that I'm missing.

I'd expect equivalence for floating point numbers to operate mostly how Java does it - if the underlying bytes are exactly the same. I would expect most of our deserializers, given the same sequence of characters, would produce the same underlying representation of the number. While I agree in practice that it's not always straightforward, or a good idea, to compare floating point numbers of arbitrary precision, I'm not convinced by what I've read thus far that it's impossible or something that would present us a particular issues in a protocol implementation.

@johnstonskj
Copy link

The EQ relation included in the IEEE standard is explicitly noted as a partial equivalence relation. It is not reflexive as there does not exist a mapping from all possible values to themselves. The real problem is not infinities it is that there is NaN values are not EQ themselves. So, it is not possible to define a full equivalence relationship for floats in the presence of NaN values.

@adamthom-amzn
Copy link
Contributor

Yes, special care would have to be paid to NaN as Java does with its equals method (specifically calling out hashtables). I don't think excluding floating point numbers from sets is practical for two reasons.

One is that customers who have no use-case for NaN values who want this functionality will be forced to reimplement this functionality themselves. I think the majority of use-cases customers are using Smithy for will not need to consider NaN. Of course this doesn't mean we don't have to consider it; I'd rather we consider it than every service owner.

The second is that this restriction, if I understand it correctly, would not just apply to sets/uniqueItems of floats, but it would also apply to collections of unique documents or structures that contain floats. While you could refuse to support sets of structures that contain a floating point member, I think it would be a surprising restriction for customers who later add a floating point member (especially if the type is shared). And, of course, with documents, they're open types where we could not do a build-time exclusion of floating point values, so you'd be left with the Rust sSDK refusing to support sets of documents containing floats at runtime, whereas other runtimes would happily accept them unless we forced them not to.

@johnstonskj
Copy link

In terms of Rust, this is solved by the crate serde_json, and then copied by others by having constructors on the the algebraic type Value that take a float (from_f64) and ensure that it supports the trait Eq and not just the default PartialEq. So, the question is whether we want to support the serialization of floats including infinities and NaNs

@mtdowling
Copy link
Member

Closed by #1106. We realized that sets of structures and unions can be dangerous for clients that drop unknown members. Given that we've now removed aggregate types from sets, it made my objections to removing float/double moot since there's no way someone could accidentally break consumers of their model by adding a float/double to a structure or union.

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

4 participants