From 9841c31058717552285984773020f15300b36893 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 12:54:33 +0000 Subject: [PATCH] fix: normalize non-dict dry-run schemas, fix ClickHouse DDL corruption, naive/aware datetime crash Consolidates three late-landing chatgpt-codex-connector review findings on PRs (#1172, #1174, #1171) that merged before the sweep could reach their branches: - benchbox/core/dryrun.py: _extract_ddl_preview called benchmark.get_schema().keys() unconditionally, but public wrapper benchmarks (e.g. JoinOrder) return a DDL string from get_schema() rather than a mapping, raising AttributeError and turning a tuned dry-run's DDL preview into a silent warning. Normalizes dict/list schemas the same way _generate_external_schema_sql() already does, falling back to an empty preview for anything else. - benchbox/core/tuning/metadata.py: the ClickHouse CREATE TABLE SQL used base_sql.replace(")", ...) to append the ENGINE clause, which rewrote every closing parenthesis in the statement -- including each VARCHAR(N) column width -- producing invalid DDL. base_sql already ends with the CREATE TABLE's closing paren, so appending the clause is sufficient and touches nothing else. - tests/uat/phases/report.py: release_gate_ordering_violations compares parsed Docker uat_lifecycle.log timestamps (naive local time, written by append_lifecycle_log) against native_stage_completed_at, which is offset-aware in production (orchestrator.py's datetime.now().astimezone(), #1162). Comparing naive and aware datetimes raised TypeError, crashing make uat-gate-check whenever a Docker stage had an action=up lifecycle entry. Normalizes the naive side onto native_stage_completed_at's awareness before comparing, preserving the existing naive-to-naive behavior other callers rely on. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- benchbox/core/dryrun.py | 11 +++++++++- benchbox/core/tuning/metadata.py | 7 +++++-- tests/uat/phases/report.py | 10 ++++++++++ tests/uat/test_report.py | 19 ++++++++++++++++++ tests/unit/core/test_dryrun.py | 20 +++++++++++++++++++ .../unit/core/tuning/test_tuning_metadata.py | 16 +++++++++++++++ 6 files changed, 80 insertions(+), 3 deletions(-) diff --git a/benchbox/core/dryrun.py b/benchbox/core/dryrun.py index 39c2ac687..fa2e5a221 100644 --- a/benchbox/core/dryrun.py +++ b/benchbox/core/dryrun.py @@ -1101,7 +1101,16 @@ def _extract_ddl_preview( if not hasattr(benchmark, "get_schema"): return ddl_preview, post_load_statements - tables = list(benchmark.get_schema().keys()) + # get_schema() is a dict on core benchmark classes but some public wrapper + # classes (e.g. JoinOrder) return a DDL string instead - normalize the same + # way _generate_external_schema_sql() does rather than assuming a mapping. + raw_schema = benchmark.get_schema() + if isinstance(raw_schema, dict): + tables = list(raw_schema.keys()) + elif isinstance(raw_schema, list): + tables = [t["name"] for t in raw_schema if isinstance(t, dict) and "name" in t] + else: + return ddl_preview, post_load_statements # Benchmark table names are lowercase (e.g. "lineitem") while shipped tuning # templates key tables uppercase (e.g. "LINEITEM"); look up case-insensitively diff --git a/benchbox/core/tuning/metadata.py b/benchbox/core/tuning/metadata.py index 75a015502..2b436a3dc 100644 --- a/benchbox/core/tuning/metadata.py +++ b/benchbox/core/tuning/metadata.py @@ -178,8 +178,11 @@ def _get_create_table_sql(self) -> str: # Redshift prefers explicit column encoding return base_sql + " ENCODE AUTO" elif platform in {"clickhouse", "clickhouse-local", "clickhouse-server"}: - # ClickHouse uses specific engine and ordering - return base_sql.replace(")", ") ENGINE = MergeTree() ORDER BY (table_name, tuning_type)") + # ClickHouse uses specific engine and ordering. base_sql always ends + # with the CREATE TABLE's closing ")", so appending here is enough - + # str.replace(")", ...) would also rewrite every VARCHAR(N) column + # width's closing paren, corrupting the column definitions. + return base_sql + " ENGINE = MergeTree() ORDER BY (table_name, tuning_type)" else: # Default for DuckDB, Databricks, etc. return base_sql diff --git a/tests/uat/phases/report.py b/tests/uat/phases/report.py index f39190bc0..ce77057ea 100644 --- a/tests/uat/phases/report.py +++ b/tests/uat/phases/report.py @@ -426,6 +426,16 @@ def release_gate_ordering_violations( violations: list[str] = [] for log_text in docker_stage_lifecycle_logs: for timestamp, platform in parse_docker_up_events(log_text): + # append_lifecycle_log() writes datetime.now() (naive local + # wall-clock time, no offset), but native_stage_completed_at is + # offset-aware in production (orchestrator.py's + # datetime.now().astimezone(), #1162). Comparing a naive and an + # aware datetime raises TypeError, so normalize a naive + # timestamp onto native_stage_completed_at's awareness -- via + # astimezone(), which interprets a naive datetime as local time + # and attaches the current local UTC offset -- before comparing. + if timestamp.tzinfo is None and native_stage_completed_at.tzinfo is not None: + timestamp = timestamp.astimezone() if timestamp <= native_stage_completed_at: violations.append( f"Docker stack '{platform}' started at {timestamp.isoformat()} " diff --git a/tests/uat/test_report.py b/tests/uat/test_report.py index 90e70651d..dc1bec8da 100644 --- a/tests/uat/test_report.py +++ b/tests/uat/test_report.py @@ -292,3 +292,22 @@ def test_release_gate_ordering_flags_docker_up_before_native_completion(): assert len(violations) == 1 assert "cedardb" in violations[0] assert "at/before native+dataframe stage completion" in violations[0] + + +def test_release_gate_ordering_does_not_raise_against_offset_aware_boundary(): + """orchestrator.py's completed_at is offset-aware (datetime.now().astimezone(), + #1162), but append_lifecycle_log() still writes naive uat_lifecycle.log + timestamps. Comparing the two used to raise TypeError; parse_docker_up_events + must normalize the naive side so the comparison (and any real violation) + still works. + """ + aware_boundary = _dt.datetime(2026, 5, 30, 1, 0, 0).astimezone() + + no_violation = report.release_gate_ordering_violations([_DOCKER_LOG_OK], native_stage_completed_at=aware_boundary) + assert no_violation == [] + + violations = report.release_gate_ordering_violations( + [_DOCKER_LOG_EARLY, _DOCKER_LOG_OK], native_stage_completed_at=aware_boundary + ) + assert len(violations) == 1 + assert "cedardb" in violations[0] diff --git a/tests/unit/core/test_dryrun.py b/tests/unit/core/test_dryrun.py index 8abdc97f4..0cd87a854 100644 --- a/tests/unit/core/test_dryrun.py +++ b/tests/unit/core/test_dryrun.py @@ -805,6 +805,26 @@ def test_missing_unified_config_returns_empty(self, tmp_path): assert ddl_preview == {} assert post_load == {} + def test_string_schema_does_not_raise(self, tmp_path): + """Public wrapper benchmarks (e.g. JoinOrder) return a DDL string from + get_schema() rather than a table_name -> columns mapping. Calling + .keys() on that string used to raise AttributeError; the preview + should instead come back empty rather than crash the dry run. + """ + unified_config = self._load_duckdb_tpch_template() + config = MagicMock() + config.options = {"unified_tuning_configuration": unified_config} + database_config = MagicMock() + database_config.type = "duckdb" + + class _StringSchemaBenchmark: + def get_schema(self) -> str: + return "CREATE TABLE title (id INTEGER, title VARCHAR);" + + ddl_preview, post_load = self.executor._extract_ddl_preview(_StringSchemaBenchmark(), config, database_config) + assert ddl_preview == {} + assert post_load == {} + def test_unknown_table_is_skipped(self, tmp_path): unified_config = self._load_duckdb_tpch_template() # Rekey the loaded template so none of its tables match a real TPC-H table. diff --git a/tests/unit/core/tuning/test_tuning_metadata.py b/tests/unit/core/tuning/test_tuning_metadata.py index 8d11d9e75..845f76c13 100644 --- a/tests/unit/core/tuning/test_tuning_metadata.py +++ b/tests/unit/core/tuning/test_tuning_metadata.py @@ -113,6 +113,22 @@ def test_create_table_sql_varies_by_platform(platform, needle): assert needle in manager._get_create_table_sql() +@pytest.mark.parametrize("platform", ["clickhouse", "clickhouse-local", "clickhouse-server"]) +def test_clickhouse_create_table_sql_does_not_corrupt_varchar_columns(platform): + """A prior str.replace(")", ...) rewrote every closing paren in the base + SQL, including each VARCHAR(N) column-width parenthesis, not just the + CREATE TABLE's final closing paren - producing invalid DDL like + 'VARCHAR(255 ENGINE = MergeTree() ... ) NOT NULL'. + """ + sql = TuningMetadataManager(_Adapter(platform_name=platform))._get_create_table_sql() + + assert "VARCHAR(255) NOT NULL" in sql + assert "VARCHAR(50) NOT NULL" in sql + assert "VARCHAR(64) NOT NULL" in sql + assert sql.count("ENGINE = MergeTree()") == 1 + assert sql.rstrip().endswith("ENGINE = MergeTree() ORDER BY (table_name, tuning_type)") + + def test_create_index_sql_skips_platforms_without_indexes(): assert TuningMetadataManager(_Adapter("clickhouse"))._get_create_index_sql() is None assert TuningMetadataManager(_Adapter("bigquery"))._get_create_index_sql() is None