chore(bigframes): refactor "generate_bigframes_bigquery.py" by sycai · Pull Request #17904 · googleapis/google-cloud-python

sycai · 2026-07-27T02:08:27Z

Changes:

Separated file parsing logic from code generation, and modularized the code
Replaced nested dictionary with data classes for better modeling of data
Let the old generate_bigframes_bigquery.py call the new Python module

The old code generation CLI still works.

I generated all the code files and checked the results. The only thing changed in the output are the relative order of AI and AEAD code

gemini-code-assist

Code Review

This pull request refactors the BigQuery code generator by modularizing the monolithic generate_bigframes_bigquery.py script into a structured package under bigquery_generator/. The feedback is highly constructive and focuses on improving the refactored code. Key recommendations include fixing the relative script path calculation to preserve descriptive headers in generated files, using relative imports for sibling modules within the new package, explicitly specifying UTF-8 encoding when opening files, adding or correcting type hints (including a type mismatch in render_accessor), and adjusting the sys.path modification to import the generator as a proper package.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…r.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…r.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

parthea · 2026-07-27T18:28:40Z

Please could you merge main into this branch to resolve the GitHub Actions Scan failure? See the fix in #17889

tswast · 2026-07-28T18:02:38Z

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+


Since this is the main entrypoint for the script, please include instructions on how to run it in a docstring.

Optionally, when run with -h or --help, also display this newly created docstring.

File removed. I will make this package un-executable on its own.

tswast · 2026-07-28T18:04:46Z

Since this is redundant with the __main__.py file you have created, please remove this file altogether and update references in our internal docs such as https://github.com/googleapis/google-cloud-python/blob/main/packages/bigframes/specs/bigframes-bigquery-generator.md with the new way to run the script.

On the second thought, I want to preserve this file while removing "main.py".

Pros:

We don't need to update the https://github.com/googleapis/google-cloud-python/blob/main/packages/bigframes/specs/bigframes-bigquery-generator.md

We won't introduce changes of script references in the generated code like this:

google-cloud-python/packages/bigframes/bigframes/operations/googlesql/aead.py

Line 17 in 4de32a5

# by the script: scripts/generate_bigframes_bigquery.py

tswast · 2026-07-28T18:06:36Z

+] + RUFF_COMMON_ARGS
+
+
+TEMPLATES: dict[str, jinja2.Template]  # Lazily-loaded global variable


It would be helpful to have a comment explaining why we want this to be lazily loaded.

tswast · 2026-07-28T18:08:01Z

Nit (Optional): We don't have to put all constants here. I would prefer that they be defined closer to where they are used or in a purpose-specific module, such as with the templates logic or list of Python built-ins, but that's mostly just a personal preference.

tswast · 2026-07-28T18:10:25Z

+from . import constants
+
+
+@dataclasses.dataclass


Ideally we'd use frozen=True on the dataclasses in this file.

Or is there a reason they need to be mutable? If so, let's document that in a comment/docstring.

tswast · 2026-07-28T19:29:50Z

+    bq_module: data_models.BQModule,
+) -> str:
+    if not bq_module.functions:
+        # If there are no function definitions, do not generate an empty file.


I'm guessing the "do not generate an empty file" part happens at a layer above this? From what I can tell here, it's returning an empty string for the file contents. I think this comment could be reworded to make that more clear (e.g. If there are no function definitions, don't generate any boilerplate so that the caller can determine that this file should be skipped.)

tswast · 2026-07-28T19:30:04Z

+
+def render_tests(bq_module: data_models.BQModule) -> str:
+    if not bq_module.functions:
+        # If there are no function definitions, do not generate an empty test file.


Same here regarding a somewhat confusing comment.

tswast · 2026-07-28T19:36:07Z

+
+def _to_snake_case(name: str) -> str:
+    # Replace dots with underscores
+    name = name.replace(".", "_")


Why the dots to underscores part? Could you provide some examples where this logic was necessary? Some parametrized unit tests or doctests could be especiallly helpful.

tswast · 2026-07-28T19:36:51Z

+    # Handle CamelCase to snake_case
+    name = re.sub(r"(?<!^)(?=[A-Z])", "_", name).lower()
+    # Replace multiple underscores with one
+    name = re.sub(r"_+", "_", name)


Same here. It would be helpful to see some concrete examples where this was needed.

tswast · 2026-07-28T19:44:45Z

+    return name
+
+
+def _parse_func_arg(arg_data: Any) -> data_models.BQFuncArg:


Nit: parse is sorta the wrong verb here. I imagine parse as the first step of going from unstructured text to some sort of structure. We're somewhere in the (symbol -> type -> resource) phases per the terminology in https://www.cs.mcgill.ca/~cs520/2018/slides/4-ast.pdf More accurate name would be _build_func_arg_ir Likewise for the other functions in this file.

sycai added 6 commits July 26, 2026 01:59

checkpoint: add logic for rendering signature template

b7e5a06

checkpoint: impl operator definition gen

cc3288e

checkout: impl test generation

8019f08

checkpoint: refactor arg rendering

310a93a

finish refactoring

5ba9eb3

make old script still runnable

ab100cb

sycai requested review from a team as code owners July 27, 2026 02:08

sycai requested review from tswast and removed request for a team July 27, 2026 02:08

sycai changed the title ~~chore (bigframes): refactor "generate_bigframes_bigquery.py"~~ chore(bigframes): refactor "generate_bigframes_bigquery.py" Jul 27, 2026

fix script reference

f85141a

gemini-code-assist Bot reviewed Jul 27, 2026

View reviewed changes

sycai and others added 16 commits July 27, 2026 02:11

fix lint

0e6d04a

Update packages/bigframes/scripts/bigquery_generator/__main__.py

9353f35

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/file_generator.py

78041aa

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/file_generator.py

4fd77b8

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/file_generator.py

aa24eb9

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/template_rendere…

393010b

…r.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/yaml_parser.py

f1fea76

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/yaml_parser.py

70e9091

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/yaml_parser.py

e1b8a37

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/generate_bigframes_bigquery.py

aa1b51b

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/template_rendere…

aa74300

…r.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/bigframes/scripts/bigquery_generator/template_rendere…

dcbd2ea

…r.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

fix lint

a048b0c

fix import

e3fb8fd

fix format

3049a97

fix lint

83b22f0

parthea assigned sycai Jul 27, 2026

sycai added 3 commits July 27, 2026 19:27

Merge branch 'main' into sycai_generator_refactor

4653ca0

Merge branch 'main' into sycai_generator_refactor

0eb0aef

update script path

ddc6d4c

tswast reviewed Jul 28, 2026

View reviewed changes

remove __main__.py from package

8ef98b9

		# See the License for the specific language governing permissions and
		# limitations under the License.

		] + RUFF_COMMON_ARGS


		TEMPLATES: dict[str, jinja2.Template] # Lazily-loaded global variable

		return name


		def _parse_func_arg(arg_data: Any) -> data_models.BQFuncArg:

Uh oh!

Conversation

sycai commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

parthea commented Jul 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sycai commented Jul 27, 2026 •

edited

Loading