Support AWS redshift queries #167
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabianfett
reviewed
Jul 25, 2021
Comment on lines
77
to
85
| case "DELETE", "UPDATE", "SELECT", "MOVE", "FETCH", "COPY": | ||
| // <cmd> rows | ||
| guard parts.count == 2 else { | ||
| // Note, Redshift does not return the actual count as per the spec | ||
| guard parts.count == 1 || parts.count == 2 else { | ||
| return nil | ||
| } | ||
| self.command = .init(parts[0]) | ||
| self.oid = nil | ||
| self.rows = Int(parts[1]) | ||
| self.rows = parts.count == 2 ? Int(parts[1]) : 0 |
There was a problem hiding this comment.
Would you mind refactoring out, the “SELECT” into its own case? Right now we would also support UPDATE and friends without row number as well.
case “SELECT” where parts.count == 1:
// AWS Redshift does not return the actual row count as defined in the postgres wire spec:
// https://www.postgresql.org/docs/13/protocol-message-formats.html in section `CommandComplete`
self.command = .init(“SELECT”)
self.oid = nil
self.rows = nil
case “DELETE”, “UPDATE”, “SELECT”, “MOVE”, “FETCH”, “COPY”:
// all the previous codeThere was a problem hiding this comment.
Yes, makes sense! Done
There was a problem hiding this comment.
I did do it different from your comment since I figured if the row count is present, we should parse it. But if that's not important (I really have no idea), I'm happy to just set rows to nil. Please let me know.
Comment on lines
19
to
23
| func testMetadataParsing() { | ||
| XCTAssertEqual(100, PostgresQueryMetadata(string: "SELECT 100")?.rows) | ||
| XCTAssertEqual(0, PostgresQueryMetadata(string: "SELECT")?.rows) | ||
| XCTAssertNil(PostgresQueryMetadata(string: "SELECT 100 100")) | ||
| } |
There was a problem hiding this comment.
Would you mind creating a new test file for this test:
PostgresNIOTests/PostgresQueryMetadataTests.swift
Since it isn’t really related to PSQLData.
fabianfett
reviewed
Jul 25, 2021
Co-authored-by: Fabian Fett <[email protected]>
fabianfett
reviewed
Jul 25, 2021
| class PSQLMetadataTests: XCTestCase { | ||
| func testSelect() { | ||
| XCTAssertEqual(100, PostgresQueryMetadata(string: "SELECT 100")?.rows) | ||
| XCTAssertEqual(0, PostgresQueryMetadata(string: "SELECT")?.rows) |
There was a problem hiding this comment.
uff, this tests needs fixing as well!
Suggested change
| XCTAssertEqual(0, PostgresQueryMetadata(string: "SELECT")?.rows) | |
| XCTAssertEqual(nil, PostgresQueryMetadata(string: "SELECT")?.rows) |
fabianfett
approved these changes
Jul 25, 2021
0xTim
approved these changes
Jul 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the metadata parser to accept a response that is missing the rowcount, event though this does not follow the spec. In my testing this allows my to now successfully query Redshift databases.
Fixes #166