[vishalmore90]

fixes #41047

Description

In Beam YAML, global aggregations across an entire PCollection can be specified without grouping keys (e.g. group_by: [] or omitting group_by).

Previously:

1. When using the Calcite / SQL provider (Combine-sql, Combine-calcite, or language: sql), _SqlCombineTransform unconditionally appended GROUP BY %s formatted with ', '.join(group_by). When group_by was empty ([]), this generated an invalid SQL query string ending with a dangling GROUP BY :

   SELECT sum(c) as total_c FROM PCOLLECTION GROUP BY 

   which Calcite rejected with a syntax parse error (SqlParseException: Encountered "<EOF>").
2. When group_by was omitted entirely from the YAML config, normalize_combine did not default group_by to an empty list, causing set(group_by) in _SqlCombineTransform to raise TypeError: 'NoneType' object is not iterable and PyJsYamlCombine.__init__ to fail on missing positional argument group_by.

This PR fixes both issues:

• _SqlCombineTransform: Defaults group_by to () and only appends the GROUP BY clause if grouping columns are present.
• normalize_combine: Defaults missing/falsy group_by configurations to [].
• PyJsYamlCombine: Defaults group_by to () and combine to {}.
• Added unit tests in yaml_combine_test.py verifying SQL query generation with grouping keys, with group_by: [], with omitted group_by, and YAML execution without group_by.
• Updated CHANGES.md.

[github-actions]

📋 Review Summary

This PR fixes a bug in YAML's SQL-backed Combine transform where omitting group_by (global aggregation) generated an invalid trailing GROUP BY clause in the SQL query. The query construction logic and unit test coverage for both PyJsYamlCombine and _SqlCombineTransform look solid.

🔍 General Feedback

• InlineProvider.config_schema_from_callable (used by generate_yaml_docs.py and per_transform schema validation) inspects parameter type annotations via typing_to_runner_api and sets nullable=True only when a parameter is annotated with Optional[...]. Adjusting the group_by and combine type annotations in PyJsYamlCombine.__init__ and _SqlCombineTransform will ensure group_by is recognized as optional and combine remains required in the generated schema and docs.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

[github-actions]

🟡 Medium: InlineProvider.config_schema_from_callable derives config field nullability (used for required in beam_schema_to_json_schema and (Optional) in generate_yaml_docs.py) directly from Optional[...] in the type annotations via typing_to_runner_api, rather than from default values.

Annotating group_by as Iterable[str] leaves group_by marked as required (nullable=False) in the generated schema/docs and conflicts with passing group_by=None. Annotating group_by as Optional[Iterable[str]] = () (matching _PyJsMapToFields in yaml_mapping.py) and keeping combine non-Optional (e.g., combine: Mapping[str, Mapping[str, Any]] = {} # pylint: disable=dangerous-default-value) ensures group_by is marked optional while combine remains required in the config schema.

Suggested change

	group_by: Iterable[str] = (),
	group_by: Optional[Iterable[str]] = (),

[chamikaramj]

Ditto.

[github-actions]

🟡 Medium: Similarly here for SqlBackedProvider.config_schema (and for type-checking the group_by=None call in test_sql_combine_query_generation), group_by should be annotated as Optional[Iterable[str]] = () and combine kept non-Optional (combine: Mapping[str, Any] = {} # pylint: disable=dangerous-default-value).

Suggested change

	group_by: Iterable[str] = (),
	group_by: Optional[Iterable[str]] = (),

[chamikaramj]

Please fix.

[github-actions]

Assigning reviewers:

R: @chamikaramj for label yaml.
Backup reviewer: derrickaw (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.48%. Comparing base (8970af1) to head (653997d).
⚠️ Report is 108 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #41049      +/-   ##
============================================
+ Coverage     57.44%   57.48%   +0.03%     
  Complexity     2492     2492              
============================================
  Files          1203     1204       +1     
  Lines        171109   171267     +158     
  Branches       2103     2103              
============================================
+ Hits          98294    98450     +156     
- Misses        70161    70163       +2     
  Partials       2654     2654              

Flag	Coverage Δ
python	80.22% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chamikaramj]

Please fix.

[chamikaramj]

Ditto.

[vishalmore90]

Hi @chamikaramj,

Thank you for reviewing my PR and for pointing this out! As a student contributor, I really appreciate the detailed guidance on how Beam YAML derives schema nullability from Optional[...] type annotations.

• Updated PyJsYamlCombine.__init__ to annotate group_by: Optional[Iterable[str]] = () and combine: Mapping[str, Mapping[str, Any]] = {}.
• Updated _SqlCombineTransform to annotate group_by: Optional[Iterable[str]] = () and combine: Mapping[str, Any] = {}.
• Disabled the dangerous-default-value pylint warning for both signatures.
• Verified that generate_yaml_docs.py now marks group_by as (Optional) and keeps combine required.
• Verified all unit tests in yaml_combine_test.py pass cleanly.

Could you please take another look when you have a moment?