You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "MonkeyCanCode (via GitHub)" <gi...@apache.org> on 2026/08/11 04:59:13 UTC
[PR] Fix DBT converts PERCENTILE_DISC to PERCENTILE_CONT on round-trip [ossie]
MonkeyCanCode opened a new pull request, #316:
URL: https://github.com/apache/ossie/pull/316
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
## Summary
Currently `_extract_agg_info` from https://github.com/apache/ossie/blob/main/converters/dbt/src/ossie_dbt/expression_utils.py#L96 is parse both `exp.PercentileCont` and `exp.PercentileDisc` into the same `(agg_type, bare_col, percentile)` which dropped the discrete flag. The rebuild later on didn't set `use_discrete_percentile` (which default to False) from https://github.com/apache/ossie/blob/main/converters/dbt/src/ossie_dbt/osi_to_msi.py#L287. Thereforce, a later MSI → OSI pass emits PERCENTILE_CONT instead of honoring original PERCENTILE_DISC when in-used.
To fix this problem, I added one extra field `use_discrete_percentile` in `_extract_agg_info` as return value.
Here is the local test validation:
```
➜ dbt git:(dbt_from_PERCENTILE_DISC_2_PERCENTILE_CONT) ✗ uv run pytest
==================================================================================================================================================== test session starts =====================================================================================================================================================
platform darwin -- Python 3.11.13, pytest-9.1.1, pluggy-1.6.0
rootdir: /Users/yong/Desktop/GitHome/ossie/converters/dbt
configfile: pyproject.toml
testpaths: tests
plugins: syrupy-5.5.3
collected 100 items
tests/test_msi_to_osi.py .................................................................. [ 66%]
tests/test_osi_to_msi.py .................................. [100%]
-------------------------------------------------------------------------------------------------------------------------------------------------- snapshot report summary ---------------------------------------------------------------------------------------------------------------------------------------------------
5 snapshots passed.
==================================================================================================================================================== 100 passed in 0.36s =====================================================================================================================================================
```
## Related Issues
<!-- Link any related GitHub issues: Closes #123, Fixes #456 -->
## Checklist
### Specification
- [ ] Spec changes are included in `core-spec/` and follow the existing structure
- [ ] Spec changes have been discussed on the mailing list or in a linked issue
- [ ] Breaking changes to the spec are clearly called out in the summary
### Ontology
- [ ] Ontology changes in `ontology/` are consistent with spec changes
- [ ] New or modified terms are defined and documented
### Converters
- [x] Converter logic in `converters/` is updated to reflect spec or ontology changes
- [x] New converters include tests under the converter's test directory
### Validation
- [ ] Validation rules in `validation/` are updated if the spec changed
- [ ] New validation cases are covered by tests
### Documentation
- [ ] `docs/` is updated to reflect any user-facing changes
- [ ] New features or behaviors are documented with examples where appropriate
- [ ] `CONTRIBUTING.md` is updated if the contribution process changed
### Examples
- [ ] `examples/` are added or updated for any new spec constructs or converter support
### Tests
- [x] All existing tests pass (`pytest` / CI green)
- [x] New functionality is covered by tests
### Compliance
- [x] ASF license headers are present on all new source files
- [x] No third-party dependencies are added without PMC/IPMC approval
--
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: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converts PERCENTILE_DISC to PERCENTILE_CONT on round-trip [ossie]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #316:
URL: https://github.com/apache/ossie/pull/316#discussion_r3945206224
##########
converters/dbt/src/ossie_dbt/expression_utils.py:
##########
@@ -36,11 +36,12 @@ def _col_name(node: exp.Expression) -> str:
return _strip_qualifier(rendered)
-def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float]]]:
+def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float], bool]]:
"""Parse a SQL aggregation expression using sqlglot.
- Returns ``(agg_type, bare_col, percentile)`` for recognised patterns, ``None`` otherwise.
- ``percentile`` is only set for ``PERCENTILE`` aggregations; it is ``None`` for all others.
+ Returns ``(agg_type, bare_col, percentile, use_discrete_percentile)`` for recognised patterns,
+ ``None`` otherwise. ``percentile`` is only set for ``PERCENTILE`` aggregations; it is ``None``
+ for all others. ``use_discrete_percentile`` is ``True`` only for ``PERCENTILE_DISC``.
The returned column name has any dataset qualifier stripped.
Review Comment:
Not in scope of this change. I can create a follow up to fix those doc-string later.
--
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: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converts PERCENTILE_DISC to PERCENTILE_CONT on round-trip [ossie]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #316:
URL: https://github.com/apache/ossie/pull/316#issuecomment-5249212448
@QMalcolm mind take a look?
--
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: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converts PERCENTILE_DISC to PERCENTILE_CONT on round-trip [ossie]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #316:
URL: https://github.com/apache/ossie/pull/316#discussion_r3945192265
##########
converters/dbt/src/ossie_dbt/expression_utils.py:
##########
@@ -36,11 +36,12 @@ def _col_name(node: exp.Expression) -> str:
return _strip_qualifier(rendered)
-def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float]]]:
+def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float], bool]]:
"""Parse a SQL aggregation expression using sqlglot.
- Returns ``(agg_type, bare_col, percentile)`` for recognised patterns, ``None`` otherwise.
- ``percentile`` is only set for ``PERCENTILE`` aggregations; it is ``None`` for all others.
+ Returns ``(agg_type, bare_col, percentile, use_discrete_percentile)`` for recognised patterns,
+ ``None`` otherwise. ``percentile`` is only set for ``PERCENTILE`` aggregations; it is ``None``
+ for all others. ``use_discrete_percentile`` is ``True`` only for ``PERCENTILE_DISC``.
The returned column name has any dataset qualifier stripped.
Review Comment:
The docstring says the returned value is a "bare_col" with any dataset qualifier stripped, but for some patterns (e.g. SUM_BOOLEAN) the second tuple element is an expression string (and may include qualifiers). This mismatch can mislead callers about what they can safely assume about the returned value.
--
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: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converts PERCENTILE_DISC to PERCENTILE_CONT on round-trip [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #316:
URL: https://github.com/apache/ossie/pull/316
--
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: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org