ennuite commented on PR #51133: URL: https://github.com/apache/arrow/pull/51133#issuecomment-5836491668
@ShivanshhhG perfect! It's normal for things to take time to wrap our head around (I've never done much C++ work before, in this or another codebase, so I'm learning as I go myself). I'm very happy you came around, it shows great character to do so. The optional semantics seem fine to me. The current test already compiles, but when I run it, it crashes with a segfault. Can you try to run it and tell me if you also hit the segfault? Do you understand why it segfaults? Hint: look at your nullptr use. Apart from the actual code correctness, the new test is not testing what we want. We want end to end tests in that file. It will help if you take a look at https://arrow.apache.org/docs/format/FlightSql.html#id3 focusing on the interaction in point 1. The idea is that the client sends the query in `DoAction(ActionCreatePreparedStatementRequest) and the server responds with `ActionCreatePreparedStatementResult{handle})`. In the server response, it will set the `is_update` field to `True` or `False`, if it's a server that has the protocol change. Old servers will not set the field, leaving it empty. For end to end tests, we want to test all 3 scenarios. Your test for `TestCommandPreparedStatementUnsetIsUpdate` is not an end to end test. It is not querying the server with a client, it is manually constructing an `ActionCreatePreparedStatementResult` and then manually constructing a client-side PreparedStatement based on that `ActionCreatePreparedStatementResult`. Compare it with `TestCommandPreparedStatementQuery` (which is an end to end test). In ``` ASSERT_OK_AND_ASSIGN(auto prepared_statement, sql_client->Prepare({}, "SELECT * FROM intTable")); ``` the client sends a `SELECT * FROM intTable to the server`, the server sends back the `ActionCreatePreparedStatementResult`and the parsing is done by the client resulting in a client-side prepared statement. This tests the whole code path, from client to server and back. To do this, you will need a server that does not set the field. Note that your change in `sqlite_server.cc` always sets the new field. Let me know if this helps and if you have further questions! I'll be AFK the next week, and I'll answer when I come back. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
