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

Fix PostgresNumeric.init crash at large number #187

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sidepelican
Copy link
Contributor

Fix #186

Fix reverseChunked to comply with maxSize.

@0xTim 0xTim requested a review from fabianfett October 11, 2021 09:49
@omochi
Copy link

omochi commented Oct 12, 2021

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 😁

@0xTim 0xTim requested a review from gwynne October 12, 2021 07:09
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

I suggest replacing both chunked() and reverseChunked() with these simpler versions:

    /// 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.
    func chunked(by maxSize: Int) -> [SubSequence] {
        return stride(from: self.startIndex, to: self.endIndex, by: maxSize).map {
            self[$0 ..< (self.index($0, offsetBy: maxSize, limitedBy: self.endIndex) ?? self.endIndex)]
        }
    }

    /// 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.
    func reverseChunked(by maxSize: Int) -> [SubSequence] {
        return stride(from: self.endIndex, to: self.startIndex, by: -maxSize).map {
            self[(self.index($0, offsetBy: -maxSize, limitedBy: self.startIndex) ?? self.startIndex) ..< $0]
        }.reversed()
    }

@sidepelican
Copy link
Contributor Author

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
Copy link
Contributor Author

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"]

@gwynne
Copy link
Member

gwynne commented Oct 14, 2021

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.

@sidepelican
Copy link
Contributor Author

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.

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.

Runtime crush at PostgresNumeric.init
3 participants