You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "MahmoudHassanen99 (via GitHub)" <gi...@apache.org> on 2026/10/09 23:52:37 UTC
[PR] feat(converters): add native ClickHouse SQL dialect converter (#546) [ossie]
MahmoudHassanen99 opened a new pull request, #548:
URL: https://github.com/apache/ossie/pull/548
<!--
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
This PR introduces a native ClickHouse SQL dialect converter under `converters/clickhouse/` as proposed in #546.
Key additions:
- **Aggregate Mappings**: Mapped native ClickHouse combinator functions (`uniqExact`, `uniqCombined64`, `quantileExact`).
- **Time Grains**: Mapped temporal truncation functions (`toStartOfDay`, `toStartOfWeek`, `toStartOfMonth`, `toStartOfQuarter`, `toStartOfYear`).
- **Safe Division**: Applied `nullIf(divisor, 0)` guards to prevent division-by-zero exceptions.
- **Tests & Quality**: Added full unit test suite under `converters/clickhouse/tests/` with 100% test coverage and validated query syntax against a live ClickHouse engine.
## Related Issues
Closes #546
## 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
- [ ] If adding a new converter, `.github/labeler.yml` is updated with the new path
### Validation
- [ ] Validation rules in `validation/` are updated if the spec changed
- [x] New validation cases are covered by tests
### Documentation
- [x] `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
- [x] If third-party source code is vendored/copied (not just declared as a dependency), `NOTICE` and/or `LICENSE` have been updated per [ASF policy](https://infra.apache.org/licensing-howto.html)
--
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] feat(converters): add native ClickHouse SQL dialect converter (#546) [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db closed pull request #548: feat(converters): add native ClickHouse SQL dialect converter (#546)
URL: https://github.com/apache/ossie/pull/548
--
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] feat(converters): add native ClickHouse SQL dialect converter (#546) [ossie]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #548:
URL: https://github.com/apache/ossie/pull/548#issuecomment-6091261029
Thank you for your first contribution to Apache Ossie (incubating)!
A committer will review your PR. In the meantime, please ensure:
- All CI checks pass
- ASF license headers are present on new source files
- Tests cover new functionality
No ICLA is required to contribute. If you are elected as a committer in the future, you will need an [Individual Contributor License Agreement (ICLA)](https://www.apache.org/licenses/contributor-agreements.html) on file with the ASF.
**Questions?** Ask in the PR comments or join us on [Slack](https://join.slack.com/t/apache-ossie/shared_invite/zt-42zw4rflt-Gpve8_NFJq7AsdAQTY~SCg).
We appreciate your contribution!
--
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] feat(converters): add native ClickHouse SQL dialect converter (#546) [ossie]
Posted by "ekut (via GitHub)" <gi...@apache.org>.
ekut commented on PR #548:
URL: https://github.com/apache/ossie/pull/548#issuecomment-6095203988
Thanks for the PR. One semantic issue in the aggregate mappings, and a suggestion for the tests.
`median` and `percentile` both map to `quantileExact`, which returns an actual value from the group and does not interpolate. The spec defines `PERCENTILE_CONT` as interpolated and `MEDIAN` as the 50th percentile [1]. Over the values 1, 2, 3, 4 on ClickHouse 26.9.5.2:
- `quantileExact(0.5)(x)` returns 3
- `quantileExactInclusive(0.5)(x)` returns 2.5, as do `PERCENTILE_CONT(0.5)` and `MEDIAN` in DuckDB 1.5.6
`quantileExactInclusive` matched DuckDB's `PERCENTILE_CONT` in all 63 combinations of group size and level I tried. On an empty group it returns `nan`; `quantileExactInclusiveOrNull` returns `NULL` there. If `percentile` is meant as `PERCENTILE_DISC`, none of `quantileExact`, `quantileExactLow` or `quantileExactHigh` matched it in every case. The value at position `max(1, ceil(level * n))` of the sorted group did.
The tests compare generated SQL strings, so they pass with either function. Running each mapping on a small fixture in ClickHouse and comparing the result with a reference engine such as DuckDB would catch this.
[1] https://github.com/apache/ossie/blob/main/core-spec/expression_language.md#percentile-functions-required
--
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] feat(converters): add native ClickHouse SQL dialect converter (#546) [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on PR #548:
URL: https://github.com/apache/ossie/pull/548#issuecomment-6333954333
@MahmoudHassanen99 the converters have moved to their own dedicated repository (https://github.com/apache/ossie-converters). Do you mind moving (creating) the PR on the new repo? Thanks!
--
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] feat(converters): add native ClickHouse SQL dialect converter (#546) [ossie]
Posted by "MahmoudHassanen99 (via GitHub)" <gi...@apache.org>.
MahmoudHassanen99 commented on PR #548:
URL: https://github.com/apache/ossie/pull/548#issuecomment-6096172400
Thanks for catching this @ekut, You're completely right
quantileExactInclusive matches the continuous percentile (PERCENTILE_CONT) interpolation required by the Ossie spec.
I have updated both median and percentile mappings to use quantileExactInclusive and updated the corresponding test suite. Pushed the fix in the latest commit
--
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] feat(converters): add native ClickHouse SQL dialect converter (#546) [ossie]
Posted by "MahmoudHassanen99 (via GitHub)" <gi...@apache.org>.
MahmoudHassanen99 commented on PR #548:
URL: https://github.com/apache/ossie/pull/548#issuecomment-6406869600
Hello christianeu-db, I don't mind
--
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