vishalmore90 commented on PR #41289:
URL: https://github.com/apache/beam/pull/41289#issuecomment-6871865814
Hi @codewarnab,
Thank you so much for the detailed review and constructive feedback!
1. **Raising `ValueError` for conflicting `value`/`config` with
expressions:**
I agree completely. If a user writes an expression like `fn: "sum(y)"`
but also passes `value: x` or `fn.config`, silently ignoring them could hide
user mistakes. Raising a descriptive `ValueError` (e.g., `ValueError(f'Cannot
specify "value" or "config" when "fn" is already a full SQL expression:
{fn_spec}')`) is much safer and clearer.
2. **`Combine-generic` and dialect checks:**
That's a great observation. `normalize_combine` runs before
language/provider expansion and handles shorthand defaults, while `combine_col`
inside `_SqlCombineTransform` acts as the defense-in-depth guard specifically
when the SQL provider is selected (including for `Combine-generic`). I will add
an explicit code comment clarifying this multi-layered protection so future
maintainers understand why both checks exist.
3. **Documenting the `'(' in fn` heuristic:**
Makes total sense. I will add a clear docstring/comment in
`yaml_combine.py` (and update `yaml-combine.md`) explaining the distinction
between a bare aggregation function name (`sum`, where the input column is
inferred from the field name) and a complete SQL expression call (`sum(col)`,
`count(*)`).
4. **Formatting config values:**
Good catch. Aggregate functions in SQL typically take numeric parameters
(e.g., percentiles) or column identifiers, but handling booleans as
`TRUE`/`FALSE` or raising an error on unsupported types makes it much more
robust.
5. **Additional test coverage:**
I will add test cases covering the `ValueError` scenario when conflicting
parameters are provided, as well as an end-to-end case through `YamlTransform`
to verify the full normalization and expansion pipeline. This should also
resolve the missing line coverage noted by Codecov.
I'd be glad to push a follow-up commit addressing these points if this
direction looks good to you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
