Skip to content

feat(converters): add native ClickHouse SQL dialect converter (#546) - #548

Open
MahmoudHassanen99 wants to merge 2 commits into
apache:mainfrom
MahmoudHassanen99:feat/clickhouse-converter-546
Open

MahmoudHassanen99 wants to merge 2 commits into
apache:mainfrom
MahmoudHassanen99:feat/clickhouse-converter-546

Conversation

@MahmoudHassanen99

Copy link
Copy Markdown

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

  • Converter logic in converters/ is updated to reflect spec or ontology changes
  • 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
  • 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

  • All existing tests pass (pytest / CI green)
  • New functionality is covered by tests

Compliance

  • ASF license headers are present on all new source files
  • No third-party dependencies are added without PMC/IPMC approval
  • If third-party source code is vendored/copied (not just declared as a dependency), NOTICE and/or LICENSE have been updated per ASF policy

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

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) on file with the ASF.

Questions? Ask in the PR comments or join us on Slack.

We appreciate your contribution!

@ekut

ekut commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

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

ekut added a commit to Dactopus/dactopus-ossie-clickhouse that referenced this pull request Oct 10, 2026
Apache Ossie's converters are named ossie-<vendor> with a module
ossie_<vendor>, and apache/ossie#548 proposes a ClickHouse one under our
current name. The package, module, command and MCP server name become
dactopus-ossie-clickhouse; the old command stays as an alias with a note
on stderr until 0.5.0. OSSIE_CLICKHOUSE_URL keeps its name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ekut added a commit to Dactopus/dactopus-ossie-clickhouse that referenced this pull request Oct 10, 2026
Apache Ossie's converters are named ossie-<vendor> with a module
ossie_<vendor>, and apache/ossie#548 proposes a ClickHouse one under our
current name. The package, module, command and MCP server name become
dactopus-ossie-clickhouse; the old command stays as an alias with a note
on stderr until 0.5.0. OSSIE_CLICKHOUSE_URL keeps its name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MahmoudHassanen99

Copy link
Copy Markdown
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][converters] Add native ClickHouse SQL dialect converter

2 participants