chore(bigquery-jdbc): update perf client to run custom queries#13521
chore(bigquery-jdbc): update perf client to run custom queries#13521logachev wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for executing SQL queries from a file (--query-file) across the JDBC client, performance runner, and associated Makefiles, alongside updating the documentation. The review feedback suggests resolving the query file to an absolute path and validating its existence early in the Python performance runner to prevent subprocess path resolution issues. Additionally, it recommends using the base name of the query file in the performance results markdown to keep columns concise, and adding a validation check in the Java client to prevent executing empty or blank queries.
| query = args.query | ||
| query_file = args.query_file | ||
| generate_rows = args.generate_rows | ||
| generate_cols = args.generate_cols |
There was a problem hiding this comment.
To ensure that relative paths for --query-file are resolved correctly regardless of the working directory of the subprocess, and to avoid running multiple benchmark iterations when the file is missing, we should convert query_file to an absolute path and validate its existence upfront.
| query = args.query | |
| query_file = args.query_file | |
| generate_rows = args.generate_rows | |
| generate_cols = args.generate_cols | |
| query = args.query | |
| query_file = args.query_file | |
| if query_file: | |
| query_file = os.path.abspath(query_file) | |
| if not os.path.isfile(query_file): | |
| import sys | |
| print(f"Error: Query file '{query_file}' does not exist.", file=sys.stderr) | |
| sys.exit(1) | |
| generate_rows = args.generate_rows | |
| generate_cols = args.generate_cols |
| new_label=new_label, | ||
| diff_label=diff_label, | ||
| spec_name=f"Rows: {args.generate_rows}, Cols: {args.generate_cols}" if args.generate_rows > 0 else args.query, | ||
| spec_name=query if query else (f"File: {query_file}" if query_file else f"Rows: {generate_rows}, Cols: {generate_cols}"), |
There was a problem hiding this comment.
Since query_file is now resolved to an absolute path, using the full path in spec_name can make the generated markdown table columns excessively wide. It is cleaner to use only the filename via os.path.basename.
| spec_name=query if query else (f"File: {query_file}" if query_file else f"Rows: {generate_rows}, Cols: {generate_cols}"), | |
| spec_name=query if query else (f"File: {os.path.basename(query_file)}" if query_file else f"Rows: {generate_rows}, Cols: {generate_cols}"), |
| if (query == null) { | ||
| System.err.println("Error: --query or --generate-rows is required when action is 'query'"); | ||
| System.err.println("Error: --query, --query-file, or --generate-rows is required when action is 'query'"); | ||
| System.exit(1); | ||
| } |
There was a problem hiding this comment.
If the user provides an empty query string (e.g., --query "") or an empty query file, the client will proceed to execute it and fail with a less descriptive JDBC driver error. Adding a check for empty/blank queries improves usability.
| if (query == null) { | |
| System.err.println("Error: --query or --generate-rows is required when action is 'query'"); | |
| System.err.println("Error: --query, --query-file, or --generate-rows is required when action is 'query'"); | |
| System.exit(1); | |
| } | |
| if (query == null) { | |
| System.err.println("Error: --query, --query-file, or --generate-rows is required when action is 'query'"); | |
| System.exit(1); | |
| } | |
| if (query.trim().isEmpty()) { | |
| System.err.println("Error: Query cannot be empty"); | |
| System.exit(1); | |
| } |
1 participant
Allow specifying custom queries via cmd-line or file for perf client