feat(smithy-swift): Big Number Support #64

sadiredd-sv · 2020-10-30T22:02:13Z

Issue #:
174932864 (from Pivotal)
Big Number Support

Description of changes:
Changing the big number library from cocoapod package to Apple numerics - https://github.com/apple/swift-numerics. This is needed because Swift SDK will be distributed for both mobile and server

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…<Double> of Swift respectively

… unit test

sadiredd-sv · 2020-11-05T10:29:15Z

...t/smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/SymbolVisitor.kt

@@ -136,12 +136,12 @@ class SymbolVisitor(private val model: Model, private val rootNamespace: String

    override fun shortShape(shape: ShortShape): Symbol = numberShape(shape, "Int16", "0")

-    override fun bigIntegerShape(shape: BigIntegerShape): Symbol = createBigSymbol(shape, "BInt")
+    override fun bigIntegerShape(shape: BigIntegerShape): Symbol = createBigSymbol(shape, "Int64")


BigInteger in smithy is "Arbitrarily large signed integer" as per https://awslabs.github.io/smithy/spec/core.html. The corresponding data type in Apple's swift-numerics (BigInt) isn't implemented in https://github.com/apple/swift-numerics.

Here are the proofs for its implementation in progress:

Arbitrary-precision bigints apple/swift-numerics#5

[Sketch]Arbitrary-precision BigInt apple/swift-numerics#84

Mentioned in this video at 11:16 in https://developer.apple.com/videos/play/wwdc2020/10217/

Alternative options:

Use this prototype of BigInt in the official Swift repository:
https://github.com/apple/swift/blob/master/test/Prototypes/BigInt.swift

Use this module which has BigInt, Complex double and Complex float:
https://github.com/dankogai/swift-pons

Use "Array(UInt64)" data structure to represent the BigInt using:
https://github.com/attaswift/BigInt

Write our own BigInt module in aws-crt-swift using the SignedInteger protocol

Since the smithy spec says that bigInteger is a Json number https://awslabs.github.io/smithy/1.0/spec/aws/aws-restjson1-protocol.html?highlight=biginteger ,

can we use Complex for BigInteger, because a Json number follows JavaScript’s 64 bit floating-point format (source: https://restfulapi.net/json-data-types/#:~:text=JSON%20numbers%20follow%20JavaScript's%20double,digits%20between%200%20and%209. )

Hm this is an interesting predicament. Okay so the best option of the ones you mentioned is one we can maintain indefinitely where we don't take a third party dependency so that's number 4, number 1 is something I previously found and is also a possibility. We want to leave room basically for apple to release something and then somehow change it to be what's in the stdlib. so we need to figure out the best way to do this. Writing our own wrapper might be the way cuz when it comes to the stdlib if it doesnt have th same name we can just keep it wrapped in our type. What do you think?

Per our discussion, we decided to wait until Apple finishes the BigInteger implementation with ComplexModule where Integer would conform to Real and their timeline would fall within our delivery of swift-sdk. So, I'm using array of bytes [UInt8] for now.

sadiredd-sv · 2020-11-05T10:36:54Z

...hy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/ShapeValueGenerator.kt

@@ -274,12 +274,12 @@ class ShapeValueGenerator(

                ShapeType.BIG_INTEGER -> {
                    writer.addImport(SwiftDependency.BIG.namespace)
-                    writer.writeInline("BInt(\$L)", node.value)
+                    writer.writeInline("(Int64)(String(\$L))", node.value)
                }


Only Double and Float conform to the Real protocol in Complex , which is available in the ComplexModule of https://github.com/apple/swift-numerics. Integer doesn't conform to this Real protocol. Hence, we can't have something like "Complex" here.

Since the smithy's big integer is a large signed integer, Int64 of Swift wont be able to handle the integer values greater than INT64_MAX and less than INT64_MIN. We cant use Complex double or float here because they don't store the "sign" of the number. So, I thought of using String, but that would imply not utilizing Apple's swift-numerics.

Apart from the above 2 reasons, the other reason why I opted to use String(node.value) is https://awslabs.github.io/smithy/1.0/spec/core/model.html?highlight=biginteger:

Question: Is this wrong ? If not, should we not cast it to Int64 at all and just use String ?

so they can be serialized as strings but can they also be deserialized as strings? that statement is a bit confusing. we need to ask the smithy folks. Seems like we still need to figure out a solution for int. How difficult would it be to conform Int to Real? Time to look into that option. We can also ask Apple if they have suggestions or guidance.

kneekey23 · 2020-11-06T15:41:21Z

...ift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGenerator.kt

@@ -61,7 +61,9 @@ fun writePackageManifest(settings: SwiftSettings, fileManifest: FileManifest, de
            writer.openBlock(".target(", "),") {
                writer.write("name: \"${settings.moduleName}\",")
                writer.openBlock("dependencies: [", "],") {
-                    writer.write(distinctDependencies.map { "\"${it.packageName}\"" }.joinToString(separator = ", "))
+                    var str = distinctDependencies.map { "\"${it.packageName}\"" }.joinToString(separator = ", ")
+                    str = str.replace("\"ComplexModule\"", ".product(name: \"ComplexModule\", package: \"swift-numerics\")")


this shouldn't be hard coded. you should be able to pull this information from the distinctDependencies array and if you cant we need to add properties to SwiftDependency

Addressed. Please check my other comment

kneekey23 · 2020-11-06T16:03:47Z

...swift-codegen/src/test/kotlin/software/amazon/smithy/swift/codegen/ImportDeclarationsTest.kt


        val statements = decls.toString()
-        val expected = "import BigNumber\nimport Foundation"
+        val expected = "import Foundation\nimport Numerics"


think this should be import ComplexModule not Numerics

Ah! my bad. Let me change this.

kneekey23

Seems like we have more research to do here. really nice start on this. We might be leaving this open for a while while we figure out what to do. Might require a baby design review with our sdk folks to see what they think. Usually defaulting to the CRT types is the answer....

…pendencies array

sadiredd-sv · 2020-11-12T10:08:39Z

...hy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/ShapeValueGenerator.kt

                ShapeType.BIG_INTEGER -> {
                    writer.addImport(SwiftDependency.BIG.namespace)
-                    writer.writeInline("BInt(\$L)", node.value)
+                    writer.writeInline("Array(String(\$L).utf8)", node.value)


This converts a big integer to array of bytes [UInt8] in Swift.

looks good to me

sadiredd-sv · 2020-11-12T10:36:36Z

...ift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGenerator.kt

+                    for(dependency in distinctDependencies) {
+                        if(dependency.packageName.equals(SwiftDependency.BIG.namespace))
+                            str = str.replace("\""+dependency.packageName+"\"", dependency.dependencyType)
+                    }


@kneekey23 , I removed the hardcoding and now retrieving the dependencies needed in targets of Package.swift from "distinctDependencies" array. I loaded the "type" field in SwiftDependency.kt because:
- Namespace's value "ComplexModule" is being used as import in ShapeValueGenerator.kt
- Dependency needed in the targets of Package.swift is different ( .product(name: "ComplexModule", package: "swift-numerics") ) from namespace

Alternative ideas:

Instead of looping over the distinctDependencies array, we can just do this:
str.replace("""+SwiftDependency.BIG.namespace+""", SwiftDependency.BIG.dependencies[0].dependencyType)

Instead of loading the "type", we can load namespace for BIG in SwiftDependency.kt with ".product(name: "ComplexModule", package: "swift-numerics") " and extract the "ComplexModule" substring for writer.addImport in ShapeValueGenerator.kt

I think im confused how is it different from namespace? the target and the namespace are the same....ComplexModule right? did I miss something, I would prefer case 2 here if I missed something but I am confused

@kneekey23 ,
Since distinctDependencies is an array of SymbolDependency objects and SymbolDependency.java doesn't have both package and namespace, I added the package name(example: swift-numerics) as property (just like the url). So, the mapping is:

namespace in SwiftDependency.kt -> packageName in Smithy's SymbolDependency.java

packageName in SwiftDependency.kt -> packageName property in Smithy's SymbolDependency.java

Is this fine?

Ah got it. So you're saying you would need to cast it to have those props from SwiftDependency right? I say just cast it and if you can't this is totally fine.

kneekey23 · 2020-11-12T17:36:20Z

...smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/SwiftDependency.kt

@@ -19,7 +19,7 @@ import software.amazon.smithy.codegen.core.SymbolDependency
 import software.amazon.smithy.codegen.core.SymbolDependencyContainer

 enum class SwiftDependency(val type: String, val namespace: String, val version: String, val url: String) : SymbolDependencyContainer {
-    BIG("", "BigNumber", "2.0", url = "https://github.com/mkrd/Swift-Big-Integer.git"),
+    BIG(".product(name: \"ComplexModule\", package: \"swift-numerics\")", "ComplexModule", "0.0.5", url = "https://github.com/apple/swift-numerics"),


ohh I think I see what you did. let's discuss this really quick and we can fix this so we can merge this in

kneekey23 · 2020-11-12T17:37:11Z

...t/smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/SymbolVisitor.kt

+    /*
+    TODO:: When https://github.com/apple/swift-numerics supports Integer conforming to Real protocol, we need to
+            change  [UInt8] to Complex<Integer>. Apple's work is being tracked in apple/swift-numerics#5
+     */


love this. we need to add another pivotal ticket for this as well and put in the icebox for now :)

I added a story in icebox #175706745

kneekey23

okay so everything looks good except the package manifest generator. let's discuss really fast to clean it up a bit and then we can merge this in.

…s loaded from SwiftDependency.kt

kneekey23

Small changes needed. Then merge this in!

kneekey23 · 2020-11-12T19:43:20Z

...ift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGenerator.kt

+                    for (dependency in distinctDependencies) {
+                        writer.openBlock(".product(", "),") {
+                            writer.write("name: \"${dependency.packageName}\",")
+                            writer.write("package: \"${dependency.expectProperty("packageName", String::class.java)}\"")


This should be the namespace property not the package name

kneekey23 · 2020-11-12T19:45:18Z

...ift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGenerator.kt

+                        writer.openBlock(".product(", "),") {
+                            writer.write("name: \"${dependency.packageName}\",")
+                            writer.write("package: \"${dependency.expectProperty("packageName", String::class.java)}\"")
+                        }


Why expect property here? And not just dependency.packageName

kneekey23 · 2020-11-12T19:46:21Z

...ift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGenerator.kt

+                            writer.write("name: \"${dependency.packageName}\",")
+                            writer.write("package: \"${dependency.expectProperty("packageName", String::class.java)}\"")
+                        }
+                    }
                }
                writer.write("path: \"./${settings.moduleName}\"")
            }


Remove commented out lines. No longer needed :)

I removed just before you commented :)

sadiredd-sv · 2020-11-12T19:53:46Z

...odegen/src/test/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGeneratorTests.kt

+                    "                .product(\n" +
+                    "                    name: \"ClientRuntime\",\n" +
+                    "                    package: \"ClientRuntime\"\n" +
+                    "                ),\n" +
                    "            ],\n" +


This is what is generated at the end (just the dependencies snippet inside targets):

Does this look good?

Yep this looks perfect 👌🏻

Adireddy added 8 commits October 30, 2020 15:00

feat(smithy-swift): Big Number Support

09ac535

feat: reverting the path changes

c3d9907

styling: with ktlintFormat

0b4f30f

feat: changed the import from BigNumber to Numerics

368e903

feat: changed the import from Numerics to ComplexModule

8bd4ff7

feat: BigNumber and BigDecimal are mapped to signed Int64 and Complex…

6430bba

…<Double> of Swift respectively

feat: used the bigdecimal datatype in kotlin for node.value and added…

40bfaa4

… unit test

styling: with ktlintFormat

55f5799

sadiredd-sv commented Nov 5, 2020

View reviewed changes

sadiredd-sv marked this pull request as ready for review November 5, 2020 19:12

sadiredd-sv requested a review from kneekey23 November 5, 2020 19:12

kneekey23 reviewed Nov 6, 2020

View reviewed changes

Adireddy added 2 commits November 12, 2020 00:10

fix: ImportDeclarationsTest fixed

61f00cc

feat: temporary placeholder for smithy's bigInteger is [UInt8] in Smithy

78db9c5

sadiredd-sv force-pushed the big-number-support branch from 684b220 to 78db9c5 Compare November 12, 2020 08:33

Adireddy added 2 commits November 12, 2020 00:37

fix: ImportDeclarationsTest and SymbolProviderTest are fixed

f67a727

fix: retrieving dependencies needed for package.swift from distinctDe…

89c5b54

…pendencies array

sadiredd-sv commented Nov 12, 2020

View reviewed changes

fix: ktlintFormat fix

c00a9c5

sadiredd-sv force-pushed the big-number-support branch from 968105c to c00a9c5 Compare November 12, 2020 10:44

sadiredd-sv requested a review from kneekey23 November 12, 2020 10:46

kneekey23 reviewed Nov 12, 2020

View reviewed changes

kneekey23 approved these changes Nov 12, 2020

View reviewed changes

feat: extracting all values from distinctDependencies array which get…

5194b23

…s loaded from SwiftDependency.kt

sadiredd-sv force-pushed the big-number-support branch from 652d342 to 5194b23 Compare November 12, 2020 19:36

fix: ktlintFormat corrected

68b2a7d

sadiredd-sv force-pushed the big-number-support branch from 567a388 to 68b2a7d Compare November 12, 2020 19:40

fix: cleaned up commented code

51a964e

sadiredd-sv force-pushed the big-number-support branch from 94c596b to 51a964e Compare November 12, 2020 19:42

kneekey23 approved these changes Nov 12, 2020

View reviewed changes

sadiredd-sv commented Nov 12, 2020

View reviewed changes

Merge branch 'swift-client-runtime' into big-number-support

ace560b

sadiredd-sv merged commit d450a9c into swift-client-runtime Nov 12, 2020

kneekey23 deleted the big-number-support branch November 16, 2020 04:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(smithy-swift): Big Number Support #64

feat(smithy-swift): Big Number Support #64

sadiredd-sv commented Oct 30, 2020 •

edited

Loading

sadiredd-sv Nov 5, 2020 •

edited

Loading

kneekey23 Nov 6, 2020

sadiredd-sv Nov 12, 2020

sadiredd-sv Nov 5, 2020 •

edited

Loading

kneekey23 Nov 6, 2020

kneekey23 Nov 6, 2020

sadiredd-sv Nov 12, 2020

kneekey23 Nov 6, 2020 •

edited

Loading

sadiredd-sv Nov 6, 2020

sadiredd-sv Nov 12, 2020

kneekey23 left a comment

sadiredd-sv Nov 12, 2020

kneekey23 Nov 12, 2020

sadiredd-sv Nov 12, 2020 •

edited

Loading

kneekey23 Nov 12, 2020

sadiredd-sv Nov 12, 2020 •

edited

Loading

kneekey23 Nov 12, 2020

kneekey23 Nov 12, 2020

kneekey23 Nov 12, 2020

sadiredd-sv Nov 12, 2020

kneekey23 left a comment

kneekey23 left a comment

kneekey23 Nov 12, 2020

kneekey23 Nov 12, 2020

kneekey23 Nov 12, 2020

sadiredd-sv Nov 12, 2020

sadiredd-sv Nov 12, 2020 •

edited

Loading

kneekey23 Nov 12, 2020

feat(smithy-swift): Big Number Support #64

feat(smithy-swift): Big Number Support #64

Conversation

sadiredd-sv commented Oct 30, 2020 • edited Loading

sadiredd-sv Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadiredd-sv Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneekey23 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneekey23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadiredd-sv Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadiredd-sv Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneekey23 left a comment

Choose a reason for hiding this comment

kneekey23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadiredd-sv Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadiredd-sv commented Oct 30, 2020 •

edited

Loading

sadiredd-sv Nov 5, 2020 •

edited

Loading

sadiredd-sv Nov 5, 2020 •

edited

Loading

kneekey23 Nov 6, 2020 •

edited

Loading

sadiredd-sv Nov 12, 2020 •

edited

Loading

sadiredd-sv Nov 12, 2020 •

edited

Loading

sadiredd-sv Nov 12, 2020 •

edited

Loading