Conversation
mattgara
requested review from
Avinash-Raj,
devavret,
karthikeyann,
misiugodfrey and
paul-aiyedun
paul-aiyedun
left a comment
paul-aiyedun
left a comment
There was a problem hiding this comment.
I had some minor comments, but changes look good to me.
presto/scripts/analyze_tables.sh
Outdated
| --host Presto coordinator hostname (default: localhost). | ||
| --port Presto coordinator port (default: 8080). | ||
| --user Presto user (default: test_user). |
There was a problem hiding this comment.
Nit: Update options to match
velox-testing/presto/scripts/run_integ_test.sh
Lines 32 to 34 in ba7cc2b
presto/scripts/analyze_tables.sh
Outdated
| ANALYZE_TABLES_SCRIPT_PATH=../testing/integration_tests/analyze_tables.py | ||
| REQUIREMENTS_PATH=../testing/requirements.txt | ||
|
|
||
| ../../scripts/run_py_script.sh -p "$ANALYZE_TABLES_SCRIPT_PATH" -r "$REQUIREMENTS_PATH" "${SCRIPT_ARGS[@]}" No newline at end of file |
There was a problem hiding this comment.
Nit: Add new line to end of file.
| analyze_tables(cursor, args.schema_name, verbose=args.verbose) | ||
| finally: | ||
| cursor.close() | ||
| conn.close() No newline at end of file |
There was a problem hiding this comment.
Nit: Add new line to end of file.
| print(f"Warning: Failed to analyze table '{table_name}': {e}") | ||
| failure_count += 1 | ||
|
|
||
| if verbose: |
There was a problem hiding this comment.
Nit: Does if verbose or failure_count > 0: also work here instead of the elif?
|
This script should be called as part of registering the tables. Is there a script for registering existing TPCH data? |
Yes, the script that handles this is setup_benchmark_tables.sh. This was added as part of @paul-aiyedun 's PR #56 which implemented Presto benchmarks.
Could you please clarify what you mean by this? I think there may be some confusion here. This PR is about adding The question you're asking seems to be about |
|
Some columns have different datatypes in the velox tpch data that we have. |
|
Okay thanks for the great feedback @karthikeyann , I've opened an issue to track this in #72. |
3 participants
This PR is intended to address #34.