Conversation
fordN
force-pushed
the
ford/clickhouse-loader
branch
2 times, most recently
from
53b0caa to
b8287fa
Compare
fordN
force-pushed
the
ford/clickhouse-loader
branch
from
b8287fa to
042ef56
Compare
- Add `make test-clickhouse` target - Fix testcontainer readiness check (remove broken wait_for_logs, rely on built-in HTTP health check) - Import container classes independently so missing driver packages don't block other containers - Fix ClickHouse test config credentials to match container defaults - Remove calls to nonexistent _record_connection_opened/_closed methods - Set supports_overwrite=False since ClickHouse only supports APPEND mode - Fix base test_batch_loading to respect supports_overwrite config
fordN
force-pushed
the
ford/clickhouse-loader
branch
from
f6b056a to
3184056
Compare
incrypto32
left a comment
incrypto32
left a comment
There was a problem hiding this comment.
LGTM! just some minor comments thats not really about this PR
| def __post_init__(self): | ||
| if self.connection_params is None: | ||
| self.connection_params = {} |
There was a problem hiding this comment.
Note for future PR: consistent with other loaders, but we should probably clean this up across the board to use field(default_factory=dict) later in another PR
| def _get_required_config_fields(self) -> list[str]: | ||
| """Return required configuration fields""" | ||
| return ['host'] # Only host is truly required, others have defaults |
There was a problem hiding this comment.
Note for future PR: All loaders could accept their typed config class directly instead of a raw dict. base class already handles the conversion internally, making this a small change per loader. Would alsoeliminate _get_required_config_fields.
| # Declare loader capabilities | ||
| SUPPORTED_MODES = {LoadMode.APPEND} | ||
| REQUIRES_SCHEMA_MATCH = False | ||
| SUPPORTS_TRANSACTIONS = False # ClickHouse uses eventual consistency |
There was a problem hiding this comment.
Minor: SUPPORTS_TRANSACTIONS = False but load_batch_transactional is implemented and will be called regardless the base class doesn't use this flag. Seems like this flag is unused? is it meant to be purely decorative/metadata?
2 participants
Load your data into a ClickHouse instance!
Summary
insert_arrow()_amp_batch_idDepends on PR #32
Resolves #5