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