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

#993 Causes ambiguous error to be thrown when result is nil #996

Closed
madisonchamberlain opened this issue Dec 8, 2023 · 13 comments
Closed
Assignees
Labels
bug Erroneous or unexpected behaviour

Comments

@madisonchamberlain
Copy link
Contributor

Hey there, I am looking for a fix for this as well, but #993 seems to have released a pretty significant bug when using the WithArrowBatches flag. I know its not released on the current version but ideally I wanted to help fix this before it is released. When I go get github.com/snowflakedb/gosnowflake@a4c355786334df0624c003ad6068ff4281cdf975 all queries which would previously return no arrow chunks are now failing with arrow/ipc: could not read message schema: arrow/ipc: could not read continuation indicator: EOF I understand that this may have been there under the hood the entire time, but previously we were still able to get results and since this isnt a snowflake error, we cant explicitly check for it and re-run the query without arrow batch. This is the code I am running. The issue is that there is no way for us to know if the query will return an empty response until we run it, and this should return with no error.

 * sqlQuery is a string of sql *
         var rows driver.Rows
	var err error
	err = conn.Raw(func(x interface{}) error {
		queryer, implementsQueryContext := x.(driver.QueryerContext)
		if !implementsQueryContext {
			return errors.NewApplication_Internal(ctx, "gosnowflake driver does not implement queryerContext")
		}
		rows, err = queryer.QueryContext(gosnowflake.WithArrowBatches(ctx), sqlQuery, nil)
		return err
	})
}

a basic query which breaks this would be select 0 where 1 = 0

go get github.com/snowflakedb/gosnowflake@cf94c15207719b43ac8199d7d183e69b9f70d4ea fixes the problem.

Another query which previously worked and does not work anymore is
``SHOW PARAMETERS LIKE 'TIMEZONE'`

The error is thrown at rows, err = queryer.QueryContext(gosnowflake.WithArrowBatches(ctx), query.Sql(), nil), not when I call fetch

@madisonchamberlain madisonchamberlain added the bug Erroneous or unexpected behaviour label Dec 8, 2023
@sfc-gh-dszmolka sfc-gh-dszmolka added the status-in_progress Issue is worked on by the driver team label Dec 11, 2023
@sfc-gh-dszmolka
Copy link
Contributor

we'll check this one too - thank you for raising the separate issue !

@sfc-gh-pfus
Copy link
Collaborator

Hello @madisonchamberlain ! First thing - it's great that you found a bug before it was even released! That saves us some problems :)

Second - I found the solution to the error that you presented when no data is returned. It was definitely my oversight in the mentioned PR. Fix: #998

Third thing - why do you use WithArrowBatches at all? When you use this mode, you can't read data in a regular manner, row based. If you enable this mode, you have to use GetArrowBatches and read data in columnar mode (example). How come that your example worked for you before? I tested it and it does not return rows, but when I switched to batches - the data was present. I tested it on master and before merging #993

@madisonchamberlain
Copy link
Contributor Author

@sfc-gh-pfus thank you for the quick fix that was fast! We are using WithArrowBatches because we send data through our system in arrow so with the arrow batches flag off, it gets converted into rows in the driver and then converted back to arrow by us so it saves us some time on each request by not doing the double transformation. I didn't include the rest of the code because it was failing before it got to this point, but the next chunk of code works like this

// type cast validation
sfRows, isSfRows := rows.(gosnowflake.SnowflakeRows)
if !isSfRows {
	return nil, errors.NewApplication_Internal(ctx, "could not cast snowflake rows to snowflake row type")
}
// null pointer dereference check, if behavior in the driver ever changed where this was null when the result set was null, we could work with that but historically this was not nil and the fetch[0] is nil
if sfRows == nil {
	return nil, errors.NewApplication_Internal(ctx, "snowflake returned nil rows and nil error")
}

arrowBatches, err := sfRows.GetArrowBatches()
if err != nil {
	return nil, wrapError(ctx, err)
}

then we loop calling sf.snowflakeArrowBatches[i].Fetch() until all of the batches have been read. Before the pr, I think rows was nil but the error was nil too. Then when we call GetArrowBatches and Fetch[0] it would return us an empty chunk which made sense in this context. Also I am not sure how this was working, but for SHOW PARAMETERS LIKE 'TIMEZONE' we were also getting a result back which had a row with a few columns. Does that answer your questions?

@sfc-gh-pfus
Copy link
Collaborator

It answers the question about using WithArrowBatches - I'm glad that someone uses it :)

About query returning parameters - I checked it. Are you sure it worked before? I checked out the commit before my changes (c962655), prepared the test which calls SHOW PARAMETERS LIKE 'TIMEZONE' and iterates over batches. But the batches collection is empty. I believe it works as designed.

Also, let's consider - what should happen, when we want to retrieve native arrow batches, but the response is JSON - should we create arrow data in driver? That doesn't make sense.

@madisonchamberlain
Copy link
Contributor Author

Yeah, it was working before. I have a unit test suite that runs on every pr which is currently on commit cf94c15207719b43ac8199d7d183e69b9f70d4ea. It runs both the show parameters like timezone and select 0 where 1=0 queries with the arrow batch flag on. Let me rewrite the test so it works with your code

@madisonchamberlain
Copy link
Contributor Author

Ok just added this test #1000

@sfc-gh-pfus
Copy link
Collaborator

Thanks @madisonchamberlain ! I ran the code you provided on the top of the commit c962655 (before any related changes were made) and it passed green, but it doesn't confirm that it worked ;) The problem is that this line:

require.Equal(t, string(params.expected[rowsRead]), col.ValueStr(i), "Unexpected result at row %d", rowsRead)

is never reached, because arrow batches are empty. Add a condition require.Equal(t, 1, len(arrowBatches)) before the loop and it will fail. As I said - in my opinion there is no chance that arrow batches work for JSON response, because we would have to create arrow structs in driver, which does not make any sense. Does it convince you?

@madisonchamberlain
Copy link
Contributor Author

Yes I am convinced thank you! I am curious what the recommended solution is here if there is one. I don't see any way to get the json response from off the rows object. If I hit this case I could rerun the query with ctx = gosnowflake.WithFetchResultByID(ctx, sfRows.GetQueryID()) but I just want to make sure I'm not missing some easy way to get the json response off the driver rows object

@sfc-gh-pfus
Copy link
Collaborator

You mean you want to receive raw JSON data, to pass it somewhere forward without deserializing, do I understand you correctly? Currently it is not available I'm afraid, we deserialize data and forget original response to free memory.

@madisonchamberlain
Copy link
Contributor Author

Ok good to know thank you!

@sfc-gh-pfus
Copy link
Collaborator

Hi @madisonchamberlain ! Can we close the issue now?

@sfc-gh-dszmolka sfc-gh-dszmolka added status-information_needed Additional information is required from the reporter and removed status-in_progress Issue is worked on by the driver team labels Dec 29, 2023
@sfc-gh-dszmolka
Copy link
Contributor

marking this one as closed for now as it seems to be sorted, but if there's still further questions please feel free to comment and we can reopen.

@sfc-gh-dszmolka sfc-gh-dszmolka removed the status-information_needed Additional information is required from the reporter label Jan 3, 2024
@madisonchamberlain
Copy link
Contributor Author

Yes, thank you for closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Erroneous or unexpected behaviour
Projects
None yet
Development

No branches or pull requests

4 participants