SQLAlchemy 2: Stop skipping some non-type tests

[susodapop]

Note to reviewers: The important thing here is that all these tests pass. The nitty gritty implementation of SQLAlchemy is both esoteric and unchanging. The key things to review are the implementation of get_pk_constraint, get_primary_keys, and the modified infer_types logic.

Background

On the path to full SQLAlchemy 2 compliance, I'm working through the entire dialect compliance test suite. Once all the compliance tests have been addressed as either passing or skipped with justification, the new dialect will be ready for release.

As of this PR, we pass 198 reviewed tests, skip 58 reviewed tests, and have not reviewed 1215 tests.

Description

This pull request represents several blocks of tests that I've reviewed in detail. I'm opening this PR as a partial effort as there are still more tests to consider. But I'd like my PR's to be smaller and more frequent rather than enormous and hard-to-review. Here is a list of the tests I enabled and anything relevant for this review.

• TableDDLTest No changes to the dialect were needed to make these pass. The table comment DDL tests are marked skip as this is not an acceptance criteria for the next release.
• HasIndexTest: Databricks doesn't support indexes. These tests are permanently skipped.
• QuotedNameArgumentTest: These tests are wacky as they are so intermingled with different requirements, some of which Databricks supports, and others which it doesn't. There's no way for us to run them as-is. We can tag this as a follow-up after the initial release.
• RowFetchTest: No changes to the dialect were needed to make this pass.
• FetchLimitOffsetTest: This failed because the default approach wrote invalid Databricks SQL (it uses -1 as the wildcard match rather than ALL). I had to implement our own sub-class of the SQLCompiler which is captured by SQLAlchemy's visitor API.
• FutureTableDDLTest: No changes to the dialect were required to make these tests pass. But I had to update requirements.py so that SQLAlchemy's test_runner would know they are runnable.
• IdentityColumnTest: I implemented IDENTITY writing semantics in the dialect to make these pass.
• HasTableTest: I had to make a few changes to bring these into passing state. I also added the reflection.cache decorator to our implementation of has_table() so that the connector will benefit from SQLAlchemy's inspector cache. This is the customary approach for first-party and third-party dialects alike. SQLAlchemy takes care of cache-invalidation for us.
• LongNameBlowoutTest: Databricks SQL doesn't support identifiers longer than 255 characters so these tests are permanently skipped.
• ExceptionTest: There is only one test in this block and it checks for an integrity error to be raised if a primary key constraint is violated. Databricks SQL doesn't enforce information constraints (apart from CHECK constraints, which are not handled inthis PR). So this is permanently skipped.
• LastrowidTest: This test depends on us implementing implicit autoincrement behaviour (where we write out an IDENTITY constraint when the user didn't specify one explicitly). This behaviour is not an acceptance criteria for the first release. The test is therefore disabled. I did add a warning log message in case users attempt to use this. The workaround is to explicitly declare Identity() for a column -- other dialects have made this similarly explicit.
• CompositeKeyReflectionTest: This is the big one: foreign and primary key reflection is a significant gap in the existing Databricks SQLAlchemy dialect. To make these tests pass I implemented the methods SQLAlchemy needs to reflect the PRIMARY KEY and FOREIGN KEY constraints defined on a given table.

Stop inferring TINYINT

After discussion with @benc-db I've also modified the pysql infer_types method so that it never automatically infers a TINYINT type. We did this because non-SELECT clauses that accept bound integer inputs (like LIMIT and OFFSET) will never accept a value that was casted to TINYINT. If users of pysql require the efficiency of a TINYINT, they can explicitly declare this type when binding their parameters.

Approach

This PR takes the same approach as #242. As I review each block of tests I mark it with @pytest.mark.reviewed. If I find that the test depends on a method existing in requirements.py, I update that file with its specific requirements. If the test cannot pass within reason for Databricks, I add it directly to test_suite.py with a rational skip marker. This means that when running pytest -m reviewed the output will look like this:

test_suite.py::IdentityColumnTest_databricks+databricks::test_select_all SKIPPED (Identity works. Test needs rewrite for Databricks. See comments in test_suite.py)
test_suite.py::IdentityColumnTest_databricks+databricks::test_select_columns SKIPPED (Identity works. Test needs rewrite for Databricks. See comments in test_suite.py)
test_suite.py::LastrowidTest_databricks+databricks::test_autoincrement_on_insert SKIPPED (This dialect does not support implicit autoincrement. See comments in test_suite.py)
test_suite.py::LastrowidTest_databricks+databricks::test_last_inserted_id SKIPPED (This dialect does not support implicit autoincrement. See comments in test_suite.py)
test_suite.py::LastrowidTest_databricks+databricks::test_native_lastrowid_autoinc SKIPPED (This dialect does not support implicit autoincrement. See comments in test_suite.py)
test_suite.py::LongNameBlowoutTest_databricks+databricks::test_long_convention_name SKIPPED (Databricks constraint names are limited to 255 characters)
test_suite.py::NumericTest_databricks+databricks::test_decimal_coerce_round_trip PASSED
test_suite.py::NumericTest_databricks+databricks::test_decimal_coerce_round_trip_w_cast PASSED
test_suite.py::NumericTest_databricks+databricks::test_enotation_decimal SKIPPED (Databricks doesn't support E notation for DECIMAL types)
test_suite.py::NumericTest_databricks+databricks::test_enotation_decimal_large SKIPPED (Databricks doesn't support E notation for DECIMAL types)
test_suite.py::NumericTest_databricks+databricks::test_float_as_decimal PASSED
test_suite.py::NumericTest_databricks+databricks::test_float_as_float PASSED
test_suite.py::NumericTest_databricks+databricks::test_float_coerce_round_trip SKIPPED (Without a specific CAST, Databricks doesn't return floats with same precision that was selected.)
test_suite.py::NumericTest_databricks+databricks::test_float_custom_scale SKIPPED (Databricks sometimes only returns six digits of precision for the generic Float type)
test_suite.py::NumericTest_databricks+databricks::test_float_is_not_numeric[Double] PASSED
test_suite.py::NumericTest_databricks+databricks::test_float_is_not_numeric[Float] PASSED

If it is possible to make a failing test pass, I implement that feature in the dialect.

[benc-db]

Style nit: If you feel like there is a concept worth extracting to a function, my preference is not to have it encapsulated inside another function, because then you can't unit test it. I would generally only use this approach for simple functions that are slightly larger than a lambda, and otherwise have the function in a scope that I can test separately. The more you can isolate units of work, the simpler your tests can be to validate their behavior.

[susodapop]

Great feedback. I'll push this to a follow-up PR shortly.

[susodapop]

Intentionally skipping failing type checks as these will be addressed in follow-ups.