postgres-nio icon indicating copy to clipboard operation
postgres-nio copied to clipboard

Fix PostgresNumeric.init crash at large number

Open sidepelican opened this issue 3 years ago • 5 comments

Fix #186

Fix reverseChunked to comply with maxSize.

sidepelican avatar Oct 11 '21 09:10 sidepelican

Hi, I am coworker with @sidepelican . We are using vapor and postgres for our company business. We depends on numeric type of postgres for accurate amount of money processing on DB. I wish to grow vapor on enterprise use-cases 😁

omochi avatar Oct 12 '21 02:10 omochi

String.Index cannot be Strideable because it needs to refer to the parent storage to know next index, so I cannot use stride. I tried to write with sequence likestride. What do you think?

sidepelican avatar Oct 14 '21 12:10 sidepelican

I tried next patterns:

let aaa: [Substring] = [
    ""[...],
    "1"[...],
    "1234"[...],
    "12345"[...],
    "12345678"[...],
    "1234567890abcd"[...],
]

for v in aaa {
    print(v.chunked(by: 4), v.reverseChunked(by: 4))
}
[""] [""]
["1"] ["1"]
["1234"] ["1234"]
["1234", "5"] ["1", "2345"]
["1234", "5678"] ["1234", "5678"]
["1234", "5678", "90ab", "cd"] ["12", "3456", "7890", "abcd"]

sidepelican avatar Oct 14 '21 12:10 sidepelican

String.Index cannot be Strideable because it needs to refer to the parent storage to know next index, so I cannot use stride. I tried to write with sequence likestride. What do you think?

Chunking a String doesn't make sense in the first place - maxSize is 3? 3 what? UTF-8 bytes? Unicode code points? Basic or extended grapheme clusters? Composed or decomposed clusters? String.Index isn't Strideable for this reason as well. Honestly, I wouldn't want these methods to work with a String or Substring. (Also, with the stride()-based implementation, you don't have to store the complete set of indices in memory (per Array(sequence(...))) before using them, though I suspect the optimizer would end up wiping out the difference anyway.)

And there's also a mistake we both made in reverseChunked():

    ///   - distance: The distance to offset `i`. `distance` must not be negative
    ///     unless the collection conforms to the `BidirectionalCollection`
    ///     protocol.
    func index(_ i: Self.Index, offsetBy distance: Int, limitedBy limit: Self.Index) -> Self.Index?

There is no check for BidirectionalCollection conformance, as rare as it is to have a Collection that isn't bidirectional. We also need to make the Strideable requirement explicit. How about this?

extension Collection where Index: Strideable, Index.Stride == Int {
    /// Split the collection into chunks of `maxSize` length each. The last chunk may contain anywhere between 1 and maxSize elements. `maxSize` must be greater than zero.
    ///
    /// Example:
    /// ```swift
    /// print(Array(1...10).chunked(by: 3)) // [[1, 2, 3], [4, 5, 6], [7, 8, 9], [10]]
    /// ```
    func chunked(by maxSize: Int) -> [SubSequence] {
        assert(maxSize > 0, "Can not chunk less than one element at a time.")
        return stride(from: self.startIndex, to: self.endIndex, by: maxSize).map {
            self[$0 ..< (self.index($0, offsetBy: maxSize, limitedBy: self.endIndex) ?? self.endIndex)]
        }
    }
}

extension BidirectionalCollection where Index: Strideable, Index.Stride == Int {
    /// Split the collection into chunks of `maxSize` length each. The **first** chunk may contain anywhere between 1 and maxSize elements. `maxSize` must be greater than zero.
    ///
    /// Example:
    /// ```swift
    /// print(Array(1...10).reverseChunked(by: 3)) // [[1], [2, 3, 4], [5, 6, 7], [8, 9, 10]]
    /// print(Array(1...11).reverseChunked(by: 3)) // [[1, 2], [3, 4, 5], [6, 7, 8], [9, 10, 11]]
    /// ```
    func reverseChunked(by maxSize: Int) -> [SubSequence] {
        assert(maxSize > 0, "Can not chunk less than one element at a time.")
        return stride(from: self.endIndex, to: self.startIndex, by: -maxSize).map {
            self[(self.index($0, offsetBy: -maxSize, limitedBy: self.startIndex) ?? self.startIndex) ..< $0]
        }.reversed()
    }
}

Of course, it's possible to write versions that work without the constraints, and a reversed version that doesn't use a negative offset, but I don't think it's worthwhile to do so.

gwynne avatar Oct 14 '21 13:10 gwynne

Sorry, I can't understand what you mean in the first sentence. I think there is some misunderstanding. At this PR, It doesn't matter whether indices in memory or calculating every time. (If you are particular about it, I will obey.) The only thing I can say is that your code using stride does not compile in my environment. Did you compile it in postgres-nio?

There is no check for BidirectionalCollection conformance,

That makes sense. nice review.

sidepelican avatar Oct 15 '21 16:10 sidepelican