Feature/jdbc #351
Feature/jdbc #351
Conversation
There was a problem hiding this comment.
Thanks for the work here @PeterLappo 😄 a couple comments:
Also looks like this PR is failing style checks, this should be resolved by running pre-commit run --all-files on your branch and pushing the resulting changes
tests/integration/test_jdbc.py
Outdated
| def create_table_row(a_str: str = "any", an_int: int = 1, a_float: float = 1.1): | ||
| return { | ||
| "A_STR": a_str, | ||
| "AN_INT": an_int, | ||
| "A_FLOAT": a_float, | ||
| } | ||
|
|
||
|
|
||
| def check_data(app_client): | ||
| response = app_client.post("/v1/statement", data=f"SELECT * from {schema}.{table}") | ||
| assert response.status_code == 200 | ||
| a_table = get_result_or_error(app_client, response) | ||
| assert "columns" in a_table | ||
| assert "data" in a_table | ||
| assert "error" not in a_table | ||
|
|
||
|
|
||
| def get_result_or_error(app_client, response): | ||
| result = response.json() | ||
|
|
||
| assert "nextUri" in result | ||
| assert "error" not in result | ||
|
|
||
| status_url = result["nextUri"] | ||
| next_url = status_url | ||
|
|
||
| counter = 0 | ||
| while True: | ||
| response = app_client.get(next_url) | ||
| assert response.status_code == 200 | ||
|
|
||
| result = response.json() | ||
|
|
||
| if "nextUri" not in result: | ||
| break | ||
|
|
||
| next_url = result["nextUri"] | ||
|
|
||
| counter += 1 | ||
| assert counter <= 100 | ||
|
|
||
| sleep(0.1) | ||
|
|
||
| return result No newline at end of file |
There was a problem hiding this comment.
Minor nitpick but could these util functions be moved above the test functions?
There was a problem hiding this comment.
I come from a Java world where the convention is to put "private" code at the end but actually coped from test_server.py which has it at the end. Shall I re-use from test_server.py? @charlesbluca
There was a problem hiding this comment.
Ah I didn't realize that this was also the case in test_server.py; in that case my preference would be to move the util functions up to the top in both files - my reasoning here being to match up with other testing files (such as test_compatibility.py) that place their utility functions at the top of the file.
I also note that since we are duplicating functions like get_result_or_error, we might want to add a testing utility module as a central location for functions like this, but that is out of the scope of this PR.
tests/integration/test_jdbc.py
Outdated
| assert "columns" in result | ||
| assert "data" in result | ||
| assert "error" not in result | ||
| assert result["columns"] == [ | ||
| { | ||
| "name": "TABLE_CATALOG", | ||
| "type": "varchar", | ||
| "typeSignature": {"rawType": "varchar", "arguments": []}, | ||
| }, | ||
| { | ||
| "name": "TABLE_SCHEM", | ||
| "type": "varchar", | ||
| "typeSignature": {"rawType": "varchar", "arguments": []}, | ||
| } | ||
| ] | ||
| assert len(result["data"]) == 3 | ||
| assert result["data"] == [ | ||
| ["", "root"], | ||
| ["", "a_schema"], | ||
| ["", "system_jdbc"], | ||
| ] |
There was a problem hiding this comment.
Wondering if there is a way to place these assertions in a helper function like assert_resp to simplify the tests; okay if this isn't possible
Remove debug code Co-authored-by: Charles Blackmon-Luca <[email protected]>
Simplify code Co-authored-by: Charles Blackmon-Luca <[email protected]>
|
@charlesbluca Updated as recommended. What is the next step to merge to main? |
|
Can one of the admins verify this patch? |
|
add to allowlist |
Co-authored-by: Charles Blackmon-Luca <[email protected]>
Co-authored-by: Charles Blackmon-Luca <[email protected]>
charlesbluca
left a comment
charlesbluca
left a comment
There was a problem hiding this comment.
Thanks for addressing my concerns @PeterLappo 🙂 would you mind merging the latest changes? Believe the last test failures were due to some changes in upstream libraries which should be resolved now
|
@charlesbluca have merged with main. Does CI pipeline run automatically? |
Codecov Report
@@ Coverage Diff @@
## main #351 +/- ##
==========================================
- Coverage 95.60% 95.48% -0.12%
==========================================
Files 67 68 +1
Lines 2932 2987 +55
Branches 547 554 +7
==========================================
+ Hits 2803 2852 +49
- Misses 79 83 +4
- Partials 50 52 +2
Continue to review full report at Codecov.
|
|
Thanks!
It normally does, but since you haven't pushed any commits to this repo your PR requires approval for CI to run - I've approved and this should be good to merge if tests pass |
4 participants
Hi,
I've added some system information to the prestodb JDBC driver so that you can see schema, table and column data in tools like dbeaver which are Java tools that use the presto JDBC driver. I'm presuming but haven't tested it that it will also work with presto ODBC drivers.
Here are a couple of dbeaver screen shots.
Thanks