Add support for Snowflake database #108
Conversation
| affected += aff | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
problem here is that we only return the last error, what we can do to remedy that is either to:
- not split on
;if that's possible and execute only one query with all the different statements - accumulate the errors
- short-circuit on the first error
There was a problem hiding this comment.
It's snowflake specific - it performs only one statement per request, so we should loop through all the statements. What i've done - i added a circuit on first error.
| } | ||
| } | ||
|
|
||
| return QueryStatus{query, query.Path, int(affected), err} |
There was a problem hiding this comment.
should we rather change the type of affected in QueryStatus to int64?
There was a problem hiding this comment.
No, I don't think so, int64 is just snowflakes' driver type of affected rows, I think we may do a type assertion to int to not change the other codebase.
sql_runner/snowflake_target.go
Outdated
| log.Fatal(err) | ||
| } | ||
|
|
||
| fmt.Println("CONFIG:", configStr) |
There was a problem hiding this comment.
Yes, i did this, it's unnecessary here
|
Thanks @BenFradet - if you could action Ben's feedback @mbobrovsky that would be great. |
| aff, _ := res.RowsAffected() | ||
| affected += aff | ||
| } | ||
| } |
There was a problem hiding this comment.
should we return an error if len(strings.TrimSpace(script)) <= 0?
There was a problem hiding this comment.
I don't think so.
Should we?
There was a problem hiding this comment.
it's always good to signal to the user that her/his input is malformed instead of silently dismissing it, no?
There was a problem hiding this comment.
Well, we can notify about the empty query string, or empty query file, but should we do it inside RunQuery() method?
There was a problem hiding this comment.
On second thought, this is just a matter of the last split being empty isn't it? In this case I think it's fine.
|
@BenFradet am I right in thinking there are still changes for @mbobrovsky to make before you give the green tick? |
|
@alexanderdean nope, just changed to approved |
|
Thanks scheduling. |
alexanderdean
changed the title
|
Hey @mbobrovsky - I started to integrate this for UA testing before I noticed that Travis is failing: The integration test suite is still using Golang 1.6 and 1.7, whereas it looks like the gosnowflake driver is only tested with 1.8 and 1.9: https://github.com/snowflakedb/gosnowflake/blob/master/.travis.yml You will need to update your PR so that tests pass (obviously). I've created a couple of tickets for you: To ensure a clean git history, you must add these into your branch before the current Snowflake commit. Does that make sense? Let me know if any questions. |
|
@alexanderdean What exactly should I do? As I undestood you commited changes on #110 , but not on #112 . Also, I can't figure out what branch is it on. |
|
Hey @mbobrovsky:
Thanks! |
|
I believe |
|
@mbobrovsky travis passes 👍 , could you rebase so that only 3 commits (corresponding to the 3 issues) are left:
|
|
@BenFradet Did I do right? |
|
Rebase to only 3 commits please |
|
Let's I create a new branch, and then create a new PR, it will be easier |
|
Go for it, closing this one |
No description provided.