Repository navigation
fix(asap-tools): give sketchdb its own SQL in ClickHouse runs #899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,7 +160,7 @@ cat > config/experiment_type/clickhouse.yaml << 'EOF' | |
| # providers.cloudlab.hostname_suffix=myexp.cloudlab.us \ | ||
| # experiment_params.dataset.name=clickbench \ | ||
| # experiment_params.dataset.local_data_file=/path/to/hits.json \ | ||
| # 'experiment_params.query_groups[0].sql_file=/path/to/queries.sql' | ||
| # 'experiment_params.query_groups[0].sql_file={baseline:/path/to/queries.sql}' | ||
|
|
||
| experiment_params: | ||
| monitoring: | ||
|
|
@@ -174,7 +174,8 @@ experiment_params: | |
| max_rows: 0 # 0 = load all rows | ||
| query_groups: | ||
| - name: default_queries | ||
| sql_file: ??? # path to SQL file generated by benchmark/generate_queries.py | ||
| sql_file: # mode -> SQL file generated by benchmark/generate_queries.py | ||
| baseline: ??? # a sketchdb mode needs its own entry (may be the same file) | ||
| repetition_delay: 0 | ||
| client_options: | ||
| starting_delay: 0 | ||
|
|
@@ -199,9 +200,12 @@ python experiment_run_clickhouse.py \ | |
| controller.punting=false \ | ||
| experiment_params.dataset.name=clickbench \ | ||
| experiment_params.dataset.local_data_file=/path/to/hits.json \ | ||
| 'experiment_params.query_groups[0].sql_file=/path/to/queries.sql' | ||
| 'experiment_params.query_groups[0].sql_file={baseline:/path/to/queries.sql}' | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This passes only
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That example runs baseline only, so it still works. bbcdac9 adds a note below it: with a sketchdb mode, give sketchdb its own entry, e.g. |
||
| ``` | ||
|
|
||
| With a `sketchdb` mode in `experiment_params.experiment`, give it its own | ||
| entry, e.g. `sql_file={baseline:/path/to/queries.sql,sketchdb:/path/to/queries.sql}`. | ||
|
|
||
| Optional overrides: | ||
| ```bash | ||
| # Skip data loading step, keep ClickHouse running after experiment | ||
|
|
@@ -239,7 +243,7 @@ python experiment_run_clickhouse.py \ | |
| experiment_type=clickhouse \ | ||
| experiment.name=frozen_plan_eval \ | ||
| controller.prebuilt_output_dir=/path/to/frozen-sql-plan \ | ||
| 'experiment_params.query_groups[0].sql_file=/path/to/queries.sql' | ||
| 'experiment_params.query_groups[0].sql_file={baseline:/path/to/queries.sql,sketchdb:/path/to/queries.sql}' | ||
| ``` | ||
|
|
||
| ### experiment_run_e2e.py (Full E2E Pipeline) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,27 +153,58 @@ def _is_clickhouse_experiment(experiment_params: DictConfig) -> bool: | |
| return "dataset" in experiment_params | ||
|
|
||
|
|
||
| def _resolve_sql_file(sql_file: Any, mode: str, idx: int, group_name: str) -> str: | ||
| """Resolve the sql_file path for one query group and mode. | ||
|
|
||
| ``sql_file`` is a dict keyed by mode name (e.g. | ||
| baseline/baseline_sketch/baseline_mv/...). sketchdb always resolves | ||
| through baseline's entry — ASAP's SQL-mode query engine must see the | ||
| exact same SQL text as baseline to pattern-match it against available | ||
| sketches, so it never gets its own dict entry. | ||
| def _mode_sql_files(query_groups: Any, mode: str) -> List[Tuple[int, Any, str]]: | ||
| """(index, group, sql_file path) of each query group `mode` runs. | ||
|
|
||
| ``sql_file`` is a dict keyed by mode name (e.g. baseline/baseline_mv/ | ||
| sketchdb). Every mode but sketchdb needs an entry in every group. | ||
| sketchdb skips the groups without one (e.g. queries ASAPQuery has no SQL | ||
| for), but only trailing groups: results are matched across modes by | ||
| global query index, so sketchdb's queries must be a prefix of baseline's, | ||
| and each file it runs must hold as many statements as baseline's. | ||
| """ | ||
| lookup_mode = ( | ||
| constants.BASELINE_EXPERIMENT_NAME | ||
| if mode == constants.SKETCHDB_EXPERIMENT_NAME | ||
| else mode | ||
| ) | ||
| path = sql_file.get(lookup_mode) | ||
| if not path: | ||
| raise ValueError( | ||
| f"Query group {idx} ({group_name!r}) missing sql_file entry for " | ||
| f"mode {lookup_mode!r}" | ||
| ) | ||
| return path | ||
| runs = [] | ||
| for idx, group in enumerate(query_groups): | ||
| name = group.get("name", str(idx)) | ||
| sql_file = group.get("sql_file") | ||
| if not sql_file: | ||
| raise ValueError(f"Query group {idx} ({name!r}) missing 'sql_file'") | ||
| if not hasattr(sql_file, "get"): | ||
| raise ValueError( | ||
| f"Query group {idx} ({name!r}) sql_file must map mode name to " | ||
| f"SQL file, e.g. {{baseline: {sql_file}}}; got {sql_file!r}" | ||
| ) | ||
| path = sql_file.get(mode) | ||
| if path: | ||
| if len(runs) != idx: | ||
| raise ValueError( | ||
| f"Query group {idx} ({name!r}) has a sql_file for mode " | ||
| f"{mode!r} after a group without one; a mode may skip " | ||
| "only trailing groups, so its query indices match baseline's" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Example: group 0 has baseline
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in bbcdac9: |
||
| ) | ||
| baseline_path = sql_file.get(constants.BASELINE_EXPERIMENT_NAME) | ||
| if ( | ||
| mode != constants.BASELINE_EXPERIMENT_NAME | ||
| and baseline_path | ||
| and os.path.exists(path) | ||
| and os.path.exists(baseline_path) | ||
| and len(_load_sql_queries(path)) | ||
| != len(_load_sql_queries(baseline_path)) | ||
| ): | ||
| raise ValueError( | ||
| f"Query group {idx} ({name!r}): mode {mode!r}'s {path!r} and " | ||
| f"baseline's {baseline_path!r} hold different numbers of " | ||
| "statements, so their query indices would not match" | ||
| ) | ||
| runs.append((idx, group, path)) | ||
| elif mode != constants.SKETCHDB_EXPERIMENT_NAME: | ||
| raise ValueError( | ||
| f"Query group {idx} ({name!r}) missing sql_file entry for " | ||
| f"mode {mode!r}" | ||
| ) | ||
| if not runs: | ||
| raise ValueError(f"No query group has a sql_file entry for mode {mode!r}") | ||
| return runs | ||
|
|
||
|
|
||
| def _validate_clickhouse_experiment_config(experiment_params: DictConfig) -> None: | ||
|
|
@@ -239,7 +270,6 @@ def _validate_clickhouse_experiment_config(experiment_params: DictConfig) -> Non | |
| "At least one mode must be defined in experiment_params.experiment." | ||
| ) | ||
| experiment_modes = {m["mode"] for m in experiment_params.experiment} | ||
| experiment_modes.discard(constants.SKETCHDB_EXPERIMENT_NAME) | ||
|
|
||
| for i, group in enumerate(experiment_params.query_groups): | ||
| sql_file = group.get("sql_file") | ||
|
|
@@ -248,9 +278,8 @@ def _validate_clickhouse_experiment_config(experiment_params: DictConfig) -> Non | |
| f"Query group {i} missing 'sql_file'. " | ||
| "Generate SQL files with benchmark/generate_queries.py first." | ||
| ) | ||
| group_name = group.get("name", str(i)) | ||
| for mode in experiment_modes: | ||
| path = _resolve_sql_file(sql_file, mode, i, group_name) | ||
| for mode in sorted(experiment_modes): | ||
| for i, _, path in _mode_sql_files(experiment_params.query_groups, mode): | ||
| if not os.path.exists(path): | ||
| raise ValueError( | ||
| f"Query group {i} sql_file for mode {mode!r} " | ||
|
|
@@ -700,6 +729,22 @@ def _is_planned(query, planned): | |
| return re.fullmatch(r"[\s()+\-*/0-9.]*", query) is not None | ||
|
|
||
|
|
||
| # A quoted time literal the query client renders, e.g. | ||
| # '{{ end_datetime[:17] }}{{ "%02d" | format(end_time % 60 // 60 * 60) }}'. | ||
| _SQL_TEMPLATED_TIME_LITERAL = re.compile(r"'\{\{[^']*\}\}[^']*'") | ||
|
|
||
|
|
||
| def _comparable_sql(sql): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Known limitation, kept as is. The saved plan holds |
||
| """`sql` without comment lines or a trailing ';', with each templated time | ||
| literal as NOW(): a saved plan holds the workload's SQL on NOW(), the | ||
| client config the same SQL on the literal the client fills in.""" | ||
| # ponytail: textual; any templated literal reads as NOW(). The engine | ||
| # matches queries to the plan structurally, this only catches a wrong plan. | ||
| lines = [line for line in sql.splitlines() if not line.lstrip().startswith("--")] | ||
| sql = _SQL_TEMPLATED_TIME_LITERAL.sub("NOW()", "\n".join(lines)) | ||
| return sql.strip().rstrip(";").strip() | ||
|
|
||
|
|
||
| def _validate_sql_inference_config(inference_config): | ||
| """Raise a clear error unless `inference_config` has the SQL schema shape.""" | ||
| if not isinstance(inference_config, dict): | ||
|
|
@@ -797,11 +842,15 @@ def copy_prebuilt_controller_output( | |
| "controller.prebuilt_output_dir inference_config.yaml has invalid " | ||
| "queries" | ||
| ) | ||
| planned = {q["query"] for q in inference_config["queries"]} | ||
| comparable = _comparable_sql if query_language == QUERY_LANGUAGE_SQL else str | ||
| planned = {comparable(q["query"]) for q in inference_config["queries"]} | ||
| with open(controller_input_config) as f: | ||
| groups = yaml.safe_load(f)["query_groups"] | ||
| unplanned = [ | ||
| q for g in groups for q in g["queries"] if not _is_planned(q, planned) | ||
| q | ||
| for g in groups | ||
| for q in g["queries"] | ||
| if not _is_planned(comparable(q), planned) | ||
| ] | ||
| if unplanned: | ||
| raise ValueError( | ||
|
|
@@ -1088,18 +1137,10 @@ def generate_clickhouse_client_configs( | |
| modes = list(mode_server_urls.keys()) | ||
| for mode, url in mode_server_urls.items(): | ||
| # Each mode may target a different sql_file (e.g. baseline_mv queries a | ||
| # materialized view, baseline_sketch swaps in a sketch function) — see | ||
| # _resolve_sql_file for the sketchdb-falls-back-to-baseline rule. | ||
| # materialized view) and sketchdb may skip trailing groups — see | ||
| # _mode_sql_files. | ||
| built_groups = [] | ||
| for idx, group in enumerate(query_groups_list): | ||
| sql_file = group.get("sql_file") | ||
| if not sql_file: | ||
| name = group.get("name", str(idx)) | ||
| raise ValueError(f"Query group {idx!r} ({name!r}) missing 'sql_file'") | ||
| sql_file = _resolve_sql_file( | ||
| sql_file, mode, idx, group.get("name", str(idx)) | ||
| ) | ||
|
|
||
| for idx, group, sql_file in _mode_sql_files(query_groups_list, mode): | ||
| queries = _load_sql_queries(sql_file) | ||
| if not queries: | ||
| raise ValueError(f"No SQL statements found in {sql_file!r}") | ||
|
|
@@ -1214,7 +1255,10 @@ def generate_sql_planner_input( | |
| groups_list = list(query_groups) | ||
|
|
||
| planner_query_groups = [] | ||
| for idx, group in enumerate(groups_list): | ||
| # The planner plans the SQL text the engine receives, i.e. sketchdb's. | ||
| for idx, group, sql_file in _mode_sql_files( | ||
| groups_list, constants.SKETCHDB_EXPERIMENT_NAME | ||
| ): | ||
| ctrl_opts = dict(group.get("controller_options") or {}) | ||
| # Dropping an unknown key here would hide it from the planner's strict parse. | ||
| unknown = set(ctrl_opts) - CONTROLLER_OPTION_KEYS | ||
|
|
@@ -1224,16 +1268,6 @@ def generate_sql_planner_input( | |
| f"{sorted(unknown)}; allowed: {sorted(CONTROLLER_OPTION_KEYS)}" | ||
| ) | ||
|
|
||
| sql_file = group.get("sql_file") | ||
| if not sql_file: | ||
| raise ValueError(f"query_groups[{idx}] missing 'sql_file'") | ||
| # sketchdb always plans against baseline's SQL text (see _resolve_sql_file). | ||
| sql_file = _resolve_sql_file( | ||
| sql_file, | ||
| constants.SKETCHDB_EXPERIMENT_NAME, | ||
| idx, | ||
| group.get("name", str(idx)), | ||
| ) | ||
| queries = _load_sql_queries(sql_file) | ||
| if not queries: | ||
| raise ValueError(f"No SQL statements found in {sql_file!r}") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,15 @@ def get_timeseries_similarity_scores( | |
| similarity_functions, | ||
| verbose: bool, | ||
| ): | ||
| # The estimate mode may run only the first queries (sketchdb skips | ||
| # trailing query groups it has no SQL for); a gap among them still fails | ||
| # on the lookup below. | ||
| if len(estimate_results) < len(queries): | ||
| print( | ||
| f"Scoring the first {len(estimate_results)} of {len(queries)} " | ||
| "queries: the estimate mode ran no others" | ||
| ) | ||
| queries = queries[: len(estimate_results)] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This truncation assumes the estimate mode ran a prefix of baseline's queries, but nothing ties the scored queries to the groups the mode actually ran; pairing goes only by count. A misconfigured Both client configs keep
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Declined for now. With the prefix rule and the per-group statement-count check (bbcdac9), the estimate's queries are exactly baseline's first N, so truncating is safe. A gap among them still fails on the lookup. Pairing by (group id, index) would change the client output and every analysis script that indexes by position. |
||
| similarity_scores = { | ||
| f.__name__: {q: 0 for q in queries} for f in similarity_functions | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template body below (~line 177) still shows
sql_file: ??? # path to SQL fileas a plain string._mode_sql_filescallssql_file.get(mode)on it and fails withAttributeError: 'str' object has no attribute 'get'instead of a clear error. The checked-inclickhouse.yamlandquantile_demo.yamlalso use a stringsql_file. Suggest updating the template to the dict form, and either rejecting a string with a clear message or accepting it as{baseline: path}.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in bbcdac9: a plain-string
sql_filenow fails with a clear error (test:test_plain_string_sql_file_is_refused_clearly), and the docs template uses the dict form. No checked-in YAML setssql_file, and there is noclickhouse.yamlorquantile_demo.yamlin the repo.