My fork is ready #376
My fork is ready #376
Conversation
|
Looks pretty good, here are my notes:
I know this seems like a lot, but please don't get discouraged. You've done a great job so far, most of the above suggestions are just intended to polish it up. Hopefully we can get it committed early next week, so it can ship in 3.27.0 on Thursday. Jeremy |
|
Thanks a lot for your careful review of my code. I am trying to refactor Connection and possible a new Statement class. Because IBM_DB does not provide initialize functions for the Connection and Statement so I cannot simply subclass them. I tried the integration test, the number of "F" almost scared me. I will catch up and update as soon as I can. |
…y default, supply "NOT NULL" to composite primary keys, correct CURRENT_TIMESTAMP zone issue
… ibm_db specific code
association_test.rbCauses of failure:
database_test.rbCause of Failure: Raw SQL "SELECT 1" is not valid for DB2. dataset_test.rbCauses of Failure:
eager_loader_test.rbFull Coverage migrator_test.rbCause of Failure: Renaming of columns are for newer versions, probably 9.7 and above. Renaming columns may work with db2 9.7 or newer. model_test.rbFull Coverage plugin_test.rbclass_table_inheritance.rb and rcte_tree.rb are not supported. prepared_statements_test.rbCauses of Failure:
schema_test.rbCauses of Failure:
timezone_test.rbDB2 only supports using UTC for database. transaction_test.rbFull Coverage type_test.rbFull Coverage. However, ibm_db has some problem dropping tables after some blob related operations, even after disconnecting and reconnect. If dropping the table manually in db2cli, then boolean type test can be passed by running rspec specifying only this test case. |
|
This is looking really good. Thanks for all of the work you are putting into it. I haven't looked at the code you, but from your notes, here are some comments:
I'll try to review the code later today when I have more time. Thanks, |
|
Thanks for all the suggestions. I will try fixing them tomorrow. For 5), you are right. It is about JOIN USING syntax. I have seen JOIN USING being used here, but nowhere else. The version of db2 in the link is old (5.4 for iSeries), and I do not know if this is just a dialect of db2 SQL. Cheers, |
|
I reviewed the code and have the following comments: You can't set @database_timezone = :utc and @application_timezone = :local in the adapter (you can do so in the DB2 adapter spec if you want). Sequel supports using multiple databases and adapters at once, and this could break the settings in other adapters. Especially the application_timezone setting to local doesn't make sense, because the user might want to use :utc as the application timezone as well. Don't raise strings as errors (Connection#prepare, Connection#execute_prepared_statement, Database#_execute). You should probably raise them as Sequel::DatabaseErrors. Connection#reorg looks vulnerable to SQL injection if table contains a '. You need to escape the table name. You should avoid using explicit return if possible:
If you override Database#alter_table, you should return whatever value is returned by super. I don't think you need to override Database#execute_dui, as the default implementation does the same thing. Database#execute probably should not rescue Exception. You should only rescue the specific exception class(es) raised by the underlying adapter. Database#execute_insert looks broken in multithreaded use, as there's no guarantee the connection used by execute_dui is the same one as the one used by metadata_dataset.get. And using a symbol inside metadata_dataset.get looks wrong. You probably want to use a literal string. Database#execute_prepared_statement should probably rescue the specific errors that can be raised and use raise_error. It also should not use eval, as that could probably be used for code injection. args should generally be ruby objects anyway, so I don't see the need for eval(literal(v)). Database#execute_prepared_statement seems to think that conn.prepared_statements[ps_name] returns a string, when you are storing a two element array of string and statement. Database#table_exists appears to be able to go into an infinite loop if the database still raises an exception. Again, you should only be rescuing the specific exception class, as currently you'll retry if an unrelated exception such as an interrupt is raised. You can probably just alias type_literal_generic_falseclass to type_literal_generic_trueclass instead of using a separate method. You should not override type_literal_specific to check for :text. Dataset#fetch_rows should remove the unless @columns and @column_info, you should not assume they will be the same as the previous call. Also, you should probably inline hash_row and use local variables. The inner part of fetch_rows is basically the inner loop when row fetching, so you should optimize it as much as possible. Take a look at some other adapters such as mysql, postgres, and sqlite to see how to optimize things. literal_false and literal_true, and the BOOL_TRUE, BOOL_FALSE, BITWISE_METHOD_MAP, and CONSTANT_MAP should move to the shared adapter. tables, views, indexes, and schema_parse_table in the shared adapter are all vulnerable to SQL injection. Use literal instead of trying to quote things yourself. Turning identifier quoting off by default is generally not advised, so you should only do that as a last resort. I know I previously advised adding a from inside get, but it's probably better to override select_from_sql and add the table there if there is no :from option set in the dataset. convert_type should be moved from the shared adapter to the native adapter. If you can't emulate OFFSET with window functions, you should probably have the dataset raise an Error instead of silently ignoring them. select_limit_sql is defined twice in the shared adapter. You should probably use #{literal(table)} inside _truncate_sql, even if identifiers are not quoted by default. You should be able to define the IBMDB_URL via an environment variable in the spec. Only database specific stuff should be tested in an adapter spec. If the spec could apply to something other than DB2, it should added to the integration tests. You should add the ibmdb adapter spec tasks to the Rakefile. You should modify the opening_databases.rdoc to add information about the ibmdb adapter. Also, modify ADAPTERS in lib/sequel/database/connecting.rb to add ibmdb. I know this seems like a lot. You've done a great job so far. I'm going to look into setting up a DB2 instance myself to test things on a regular basis, so that I can be sure support for DB2 stays good. |
|
I pushed some commits today that make it possible for extract to work correctly (see the shared sqlite and mssql adapters), and should fix the where 1 issue (set Dataset#supports_where_true? to false). |
|
I got DB2 setup in a VM, and I'm currently working with your branch. With only a small patch to enable the specs and fix a small syntax error (http://pastie.org/2523170), I'm getting "363 examples, 7 failures, 25 pending", which is a good sign. I don't want to overlap with what your doing, but I'm happy to test patches now, and if you get stuck in any part, I can make modifications and test them. |
|
Here's a diff to your adapter with some small changes for better compatibility: http://pastie.org/2523585 |
|
Sorry for being not responsive during the last few days. I am just back from vacation and thanks a lot for your careful review and help. I made some changes according to your comments, but it is still not complete. Below are the things that you may want to know.
It is a great help that you can setup a db2 instance yourself, and looks like your db2 version is newer than mine (9.1) and produces less errors. |
|
Your changes look good. I'll merge your changes into my local branch and just keep working on things. After things are mostly working, I'll merge it into the main Sequel branch, and after that we can just make small fixes to the main branch to fix issues. I'll need your help in testing older versions of DB2, since I don't want to setup multiple versions to test on. Thanks to your help with the shared DB2 adapter, I'm also working on changes to the main db2 adapter (which uses ruby-db2) that make it actually run. Additionally, I'm working on a JDBC subadapter for DB2, but not much luck on that yet. |
|
I've almost got something ready to commit. I'm going to review the full diff from master tonight, and should be committing things tomorrow. |
|
Thanks for all of your help, Roy. If you find any issues with the changes I made that break something in DB 9.1, please send a pull request that makes some things conditional on version (you'll probably want to turn db2_version into an integer so it's easy to compare (server_version < 9070401). These are the only currently 6 pending specs on 9.7, everything else passes: The OFFSET emulation with ROW_NUMBER doesn't seem to like a prepared statement, though it works fine in a regular query. The rest of these are just issues with DB2, and probably won't be fixed. FWIW, ibmdb is one of the best adapters in terms of passing specs. postgres is the best with 0, followed by mysql with 1, h2 with 3, and then mysql2 and ibmdb with 6. |
|
That is really a huge commit, and the code looks a lot clearer. I have tried integration test here, and everything works fine. The failed ones here are all expected so there is no need to make anything conditional on version. Please make change to Database#db2_version to return integer if you like. Freeing prepared stmt in Database#excute_prepared_statement worries me a little. I am not sure if a statement can be reused after being freed. Perhaps it will give out error when for the second time it is executed with new bind arguments. Also, although all prepared statement tests pass, prepared statement function does not seem to work as expected here. |
|
That's not a bug, the :where option is ignored when inserting (what would it mean)? I'm guessing you want: |
|
About freeing the prepared statement, it calls |
|
Freeing the statement does no harm. I was wrong. |
Please review my code and make any change if you want to. I can pull back and do spec testing here. Thanks.