Skip to content

isthmus: a UserTypeMapper's reverse direction only sees user-defined types, so a mapped built-in cannot come back #1219

Description

@nielspardon

UserTypeMapper.toSubstrait returns any Type, and TypeConverter consults it before its own switch precisely so a mapper can claim a SqlTypeName the switch already handles — the call site says so:

private Type toSubstrait(RelDataType type, List<String> names) {
  // Check for user mapped types first as they may re-use SqlTypeNames
  Type userType = userTypeMapper.toSubstrait(type);

Mapping a Calcite built-in onto a Substrait built-in is therefore part of the forward contract rather than an accident of the signature: were the mapper restricted to user-defined types, checking it first would buy nothing, since a user-defined type's SqlTypeName is not one the switch handles and would fall through to default either way.

The reverse direction does not follow. toCalcite takes a Type.UserDefined, and TypeConverter.ToRelDataType calls it from exactly one place, visit(Type.UserDefined); every other visit builds a Calcite type from the factory with no hook. So a mapped built-in has no way back:

Calcite CHAR(1) -> mapper -> Type.Str -> ToRelDataType.visit(Type.Str) -> VARCHAR, PRECISION_NOT_SPECIFIED

A column that went in as CHAR(1) comes back as an unparameterised VARCHAR, and the mapper that made the forward decision is never asked. The consumer #1170 was filed for maps Impala's string — which reaches Calcite as VARCHAR(2147483647) — onto Substrait's string, so this is the shape the forward direction is actually used in.

Two ways to close it, and they are not equivalent. Widening toCalcite to take a Type and consulting it from every ToRelDataType.visit keeps the forward contract and makes the round trip the mapper's own responsibility. Narrowing toSubstrait to Type.UserDefined would make the two signatures agree instead, but it contradicts the mapper-first ordering above and removes the use #1170 was filed for. The interface's class Javadoc — "Defines conversion of user-defined types between Substrait and Calcite" — reads as though the second were already the contract, so it needs updating either way.

Found while reviewing #1171, whose description raises the same asymmetry from the forward side. #1190 covers the separate problem that most literal families ignore the mapper's answer. Checked on main @ 934a60e.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions