Skip to content

Commit 789c4f5

Browse files
eddietejedaclaude
andauthored
fix: code quality review fixes (#8)
* fix: always include schema in managed config even when tables list is empty build_managed_config was returning {} when tables=[], silently dropping the schema name. A call like create_database("db", schema="analytics") would send config: {} to the API and the schema declaration was lost. Remove the early return so the schema block is always emitted. Add a regression test for the empty-tables path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: remove duplicate obj/schema validation from create_table The check was already present in _local_table_to_parquet with the same condition and message. Having it in create_table too was dead code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: add missing client.close() in test_result_arrow_poll_handles_accepted_result Every other HotdataClient created in this file is closed at the end of the test. This one was missed, leaving a dangling connection pool. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: explain _managed_table_synced behavior for pending-sync tables The method returns False for tables whose synced flag is False, which lets create_table proceed without overwrite=True while a load is still in progress. This was intentional but undocumented, making the behavior look like a bug. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * nit: document why except Exception is intentionally broad in dtype_from_hotdata_sql_type ibis and sqlglot raise several different exception types depending on which part of type parsing fails (ValueError, AttributeError, parse errors internal to sqlglot). Narrowing to a specific type risks missing one and breaking type discovery for a valid-but-unusual column type. Add a comment so the broad catch is clearly deliberate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent fcc3270 commit 789c4f5

5 files changed

Lines changed: 41 additions & 6 deletions

File tree

src/ibis_hotdata/backend.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,13 @@ def _managed_table_synced(
345345
schema_name: str,
346346
table_name: str,
347347
) -> bool:
348+
"""Return True only if the table exists and its last load has completed.
349+
350+
A table whose ``synced`` flag is False is still being loaded; we treat
351+
it as writable (returns False) so that an in-progress load can be
352+
retried without requiring ``overwrite=True``. Tables not present in the
353+
information schema also return False (not yet created).
354+
"""
348355
for row in self._iterate_information_schema(
349356
{"connection_id": connection_id, "schema": schema_name, "table": table_name},
350357
include_columns=False,
@@ -613,9 +620,6 @@ def create_table(
613620
if temp:
614621
raise NotImplementedError("Hotdata does not support temporary tables.")
615622

616-
if obj is not None and schema is not None:
617-
raise com.IbisInputError("create_table accepts only one of obj or schema")
618-
619623
data = self._local_table_to_parquet(obj, schema)
620624
connection_id, schema_name = self._table_location(database)
621625
if not overwrite and self._managed_table_synced(connection_id, schema_name, name):

src/ibis_hotdata/managed.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@
99

1010

1111
def build_managed_config(schema: str, tables: list[str]) -> dict[str, Any]:
12-
if not tables:
13-
return {}
1412
return {
1513
"schemas": [
1614
{

src/ibis_hotdata/types.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,5 +12,5 @@ def dtype_from_hotdata_sql_type(sql_type: str | None, *, nullable: bool) -> dt.D
1212
return dt.String(nullable=nullable)
1313
try:
1414
return PostgresType.from_string(sql_type.strip(), nullable=nullable)
15-
except Exception:
15+
except Exception: # ibis/sqlglot raise a variety of parse errors; fall back to String
1616
return dt.String(nullable=nullable)

tests/test_hotdata_backend.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,38 @@ def on_create(req: Request) -> Response:
499499
con.create_database("sales", schema="public", tables=["orders"])
500500

501501

502+
def test_create_database_no_tables_still_sends_schema(httpserver: HTTPServer, srv: str):
503+
def on_create(req: Request) -> Response:
504+
body = req.get_json()
505+
assert body == {
506+
"name": "empty_db",
507+
"source_type": "managed",
508+
"config": {
509+
"schemas": [{"name": "analytics", "tables": []}],
510+
},
511+
"skip_discovery": True,
512+
}
513+
return Response(
514+
json.dumps(
515+
{
516+
"id": MANAGED_CONN,
517+
"name": "empty_db",
518+
"source_type": "managed",
519+
"discovery_status": "skipped",
520+
"tables_discovered": 0,
521+
}
522+
),
523+
status=201,
524+
content_type="application/json",
525+
)
526+
527+
httpserver.expect_request("/v1/connections", method="GET").respond_with_json({"connections": []})
528+
httpserver.expect_request("/v1/connections", method="POST").respond_with_handler(on_create)
529+
530+
con = ibis.hotdata.connect(api_url=srv, token="tok", workspace_id="ws", verify_ssl=False)
531+
con.create_database("empty_db", schema="analytics")
532+
533+
502534
def test_drop_table_deletes_managed_table(httpserver: HTTPServer, srv: str):
503535
httpserver.expect_request("/v1/connections").respond_with_json(managed_connections_response())
504536
httpserver.expect_request(

tests/test_hotdata_http.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ def test_result_arrow_poll_handles_accepted_result(httpserver: HTTPServer):
124124
)
125125
out = client.execute_query("select 1", poll_interval_s=0, poll_timeout_s=5)
126126
assert out["pa_table"].to_pydict() == {"n": [42]}
127+
client.close()
127128

128129

129130
def test_async_query_run_failure(httpserver: HTTPServer):

0 commit comments

Comments
 (0)