Fix ILLEGAL_TYPE_OF_ARGUMENT on nullable MySQL spatial columns

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: data-structures, error-handling, backwards-compatibility, testing-strategy | Difficulty: mid
Link: https://github.com/ClickHouse/ClickHouse/pull/110943

The problem

When reading nullable MySQL spatial columns (such as LINESTRING, POLYGON, etc.) using ClickHouse's MySQL integration, schema inference failed with an ILLEGAL_TYPE_OF_ARGUMENT error. This was because ClickHouse's internal geometric types (like LineString) cannot be wrapped inside a Nullable type, but the code tried to do so unconditionally. As a result, users could not query or describe tables with nullable spatial columns, which is the default in MySQL.

Why it's non-trivial

A naive fix might try to simply allow wrapping any type in Nullable, but some types in ClickHouse (notably the array/variant-based geometric types) explicitly do not support being nested inside Nullable due to type system invariants. Hardcoding a list of which types can/cannot be wrapped would be brittle and error-prone, especially as new types are added. The fix must be generic and future-proof, and preserve the ability to read real NULL values from MySQL without silently converting them to empty or default geometries.

The approach

1. Guard the Nullable wrap with a property check

Previously, the code always wrapped the mapped type in Nullable if the MySQL column was nullable:

-    if (is_nullable)
-        res = std::make_shared<DataTypeNullable>(res);

This was changed to only wrap types that can actually be inside Nullable, using the canBeInsideNullable() property:

    if (is_nullable)
    {
        /// A mapped type that cannot be inside `Nullable` (the `Array`/`Variant`-based geometric
        /// types) falls back to `Nullable(String)` holding MySQL's own bytes (SRID prefix + WKB),
        /// matching the result-set path.
        if (res->canBeInsideNullable())
            res = std::make_shared<DataTypeNullable>(res);
        else
            res = std::make_shared<DataTypeNullable>(std::make_shared<DataTypeString>());
    }

• What this does:
  • If the mapped type supports being inside Nullable, wrap it as before.
  • If not, fall back to Nullable(String), storing the raw MySQL bytes (SRID + WKB), which is a safe and compatible fallback.

2. Documentation and settings update

The documentation for the relevant setting was updated to clearly describe the new fallback behavior, including the case for nullable spatial types:

-... spatial types ... to the corresponding ClickHouse geometric types ... 
+- ... spatial types ... to the corresponding ClickHouse geometric types ... 
+  A nullable spatial column other than `POINT` also maps to `Nullable(String)`, since `Point` is the only geometric type that can be nested inside `Nullable`. Every such string is a 4-byte SRID prefix followed by the WKB payload, as returned by MySQL. ...

3. Integration test added

A new integration test was added to verify:

• Nullable spatial columns infer as Nullable(String)
• The exact bytes returned match MySQL's output
• NULL values are preserved as NULLs
• POINT columns (which can be inside Nullable) still map to Nullable(Point)

Key test assertion:

assert (
    node1.query(
        "SELECT toTypeName(ls), toTypeName(pg), toTypeName(mls), toTypeName(mpg), toTypeName(mpt), toTypeName(geo) "
        f"FROM {table_function} LIMIT 1"
    ).strip()
    == "\t".join(["Nullable(String)"] * 6)
)

• What this does:
  • Confirms that all the tested spatial columns (except POINT) are inferred as Nullable(String).

Another key assertion ensures POINT is handled correctly:

assert (
    node1.query(f"SELECT toTypeName(pt) FROM {table_function} LIMIT 1").strip()
    == "Nullable(Point)"
)

From the review thread

No notable review discussion on this PR.

Takeaways

• Type system invariants must be enforced at all abstraction layers: Never wrap a type in a container (like Nullable) unless it explicitly supports it. Use property checks (e.g., canBeInsideNullable()) rather than hardcoded type lists.
• Fallbacks should preserve user data and intent: When a type mapping cannot be preserved, fall back to a representation (like Nullable(String)) that safely carries the original data, including NULLs, rather than erroring or silently converting values.
• Regression tests should cover both error and success cases: When fixing a bug, add integration tests that assert both the absence of the previous error and the correct handling of edge cases (e.g., NULLs, exact byte values, and type inference).