feat: add GetStatistics test#187
Conversation
|
|
||
|
|
||
| class TestConnection: | ||
| @staticmethod |
There was a problem hiding this comment.
We can just use a constant at global level - this isn't Java :)
| if not driver.features.connection_get_statistics: | ||
| pytest.skip("not implemented") |
There was a problem hiding this comment.
It would be better to do this in generate_tests since currently the fixture will run even if the driver doesn't support this
| if not hasattr(conn, "adbc_get_statistics"): | ||
| pytest.skip("adbc_driver_manager DBAPI does not expose GetStatistics") |
There was a problem hiding this comment.
IMO this should be an error, why enable the test and then silently skip it?
| ) | ||
|
|
||
| # Verify statistic_is_approximate is a boolean | ||
| assert isinstance(stat["statistic_is_approximate"], bool), ( |
There was a problem hiding this comment.
The data is typed, and we validated the Arrow schema above, so there's no point validating this
| # Verify statistic value is a scalar (int, float, bytes, or None) | ||
| assert isinstance( | ||
| stat["statistic_value"], (int, float, bytes, type(None)) | ||
| ), ( | ||
| f"statistic_value should be int|float|bytes|None, " | ||
| f"got {type(stat['statistic_value'])}" | ||
| ) |
| f"got {type(stat['statistic_value'])}" | ||
| ) | ||
|
|
||
| # If row count statistic is present, verify it's reasonable since approx = true |
There was a problem hiding this comment.
I think this works OK to start but it may be good to extend DriverFeatures with a list of expected statistic names, sort of like with xdbc
There was a problem hiding this comment.
sounds good. we can merge this first and tackle this later with seperate PR.
Co-authored-by: David Li <li.davidm96@gmail.com>
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
LGTM, we can merge once adbc-driver-manager is updated
2 participants
What's Changed
apache/arrow-adbc#4129 need to be merged in first.
Closes #182