[#11910] fix(clickhouse): fix GraphiteMergeTree classification and add missing type mappings#11915
Open
jiangxt2 wants to merge 1 commit into
Open
[#11910] fix(clickhouse): fix GraphiteMergeTree classification and add missing type mappings#11915jiangxt2 wants to merge 1 commit into
jiangxt2 wants to merge 1 commit into
Conversation
yuqi1129
reviewed
Jul 8, 2026
| // No type mapping for date32, we will use external type to handle it. | ||
| case DATE32: | ||
| // Date32 supports 1900-2299 vs Date's 1970-2149. Use ExternalType to preserve round-trip. | ||
| return Types.ExternalType.of(DATE32); |
Contributor
There was a problem hiding this comment.
We haven't handled most types that can't be converted to the Gravitino type, as engines like Trino will not recognize externalType. Adding this type is designed to display types that are created by the catalog itself in Gravitino. As for how to use this type uniformly in engines, there is no clear solution now. anyway, this PR is useful and valuable.
Contributor
|
Adding some ITs is preferable. |
Code Coverage Report
Files |
…and add missing type mappings
Fix GraphiteMergeTree engine incorrectly classified as not requiring
ORDER BY, and add explicit type mappings for ClickHouse types that
previously fell through to generic ExternalType.
**GraphiteMergeTree engine fix**
- Change ENGINE enum from single-arg to three-arg constructor
(requireOrderBy=true, acceptPartition=true)
- Add graphite.config property support for GraphiteMergeTree DDL
generation (required by ClickHouse: ENGINE = GraphiteMergeTree('config'))
- Register GRAPHITE_CONFIG_PROPERTY_ENTRY in table properties metadata
**Enum8/Enum16 normalization**
- Add startsWith('Enum') guard before the type switch to normalize
Enum8/Enum16 to base ENUM constant, preserving full parameterized
form as ExternalType
- Use stripped typeName (not typeBean.getTypeName()) to avoid
Nullable/LowCardinality wrapper leakage
**New type mappings**
- Int128/Int256/UInt128/UInt256 → ExternalType (wide integers)
- BFloat16 → ExternalType (no half-precision in Gravitino)
- Date32 → ExternalType (wider range than Date)
**ClickHouseContainer JDBC URL fix**
- Use localhost + getMappedPort() instead of container internal IP
for macOS Docker Desktop compatibility
**IT tests**
- testCreateTableWithNewTypeMappings: verify Int128/256, UInt128/256,
Enum8/16, Date32 round-trip via SQL CREATE TABLE + loadTable
- testGraphiteMergeTreeEngineCreation: verify missing graphite.config
property throws IllegalArgumentException
- BFloat16 excluded from IT (requires ClickHouse 24.12+, CI uses 24.8.14)
Signed-off-by: jiangxt2 <jiangxt2@vip.qq.com>
jiangxt2
force-pushed
the
fix/clickhouse-graphite-typedef
branch
from
a2deb3a to
aea1b99
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: #11910
What changes were proposed in this pull request?
Why are the changes needed?
GraphiteMergeTree tables cannot be created through Gravitino at all — the engine requires ORDER BY but the catalog rejects it. Multiple ClickHouse types (Enum8/16, wide integers, BFloat16, Date32) fall through to generic ExternalType instead of being handled explicitly.
Does this PR introduce any user-facing change?
No. GraphiteMergeTree tables become creatable; other type mappings preserve existing ExternalType behavior.
How was this patch tested?
./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test -PskipITs— all 46 unit tests pass