Skip to content

isthmus: LiteralConverter derives every literal but the character ones from the Calcite type, so a UserTypeMapper is dropped for the other families #1190

Description

@nielspardon

LiteralConverter.convert(RexLiteral, RelDataType) derives the Substrait type through the UserTypeMapper and then throws that answer away for most literal families:

https://github.com/substrait-io/substrait-java/blob/5ce7fb94/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java#L112-L122

type on line 114 is the mapped type — TypeConverter.toSubstrait consults userTypeMapper.toSubstrait before its own switch — and it is what toNamedStruct puts in the schema. The switch on line 122 then dispatches on resultType.getSqlTypeName() and rebuilds the literal from the Calcite type a second time, so the literal and the schema disagree wherever the mapper changed the answer. #1170 is the character-typed instance of this and #1171 routes CHAR/VARCHAR through the derived type; every other arm still re-derives.

The arms that ignore type: TINYINT/SMALLINT/INTEGER/BIGINT (123-131) and BOOLEAN (131) and FLOAT/DOUBLE/REAL (142-145) and SYMBOL (172) and DATE (190) hard-code the literal form; DECIMAL (148) reads resultType.getPrecision() and getScale(); BINARY (162) reads resultType.getPrecision() for both the form and the zero-pad width; TIME (196) and TIMESTAMP/TIMESTAMP_WITH_LOCAL_TIME_ZONE (204) read TypeConverter.precisionOf(resultType), and the latter picks between precisionTimestamp and precisionTimestampTZ from the SqlTypeName (222); INTERVAL_YEAR (226) hard-codes intervalYear, ignoring a mapping to intervalCompound; INTERVAL_DAY (235) reads resultType.getScale(); ROW (272) and ARRAY (283) always build a struct or a list regardless of what the mapper returned for the container, and recurse on the Calcite field and component types.

Two tiers of consequence, and the second is the reason to treat this as more than a tidy-up. Mapping INTEGER to i64 — the most likely mapper anyone writes — gives schema i64 and literal i32, and on the LogicalValues path VirtualTableScan's @Value.Check rejects it loudly with Row field type (I32{nullable=false}) does not match schema field type (I64{nullable=false}), the same symptom #1170 was filed with. But for DECIMAL and the temporal families the divergent number is not just a type parameter, it is the unit the value is encoded in, so the plan is wrong rather than inconsistent. ExpressionCreator.decimal encodes the value bytes at the scale it is handed, so mapping DECIMAL(10,2) to decimal(38,10) writes unscaled 125 for 1.25 and tags it decimal<10,2> while the schema says decimal<38,10>; a consumer reading those bytes at the declared scale of 10 gets 0.0000000125. INTERVAL_DAY is the same shape and harder to spot, because the spec defines the sub-second component as "expressed in 1e(-P) units" (type classes), so mapping INTERVAL DAY TO SECOND(3) to intervalDay(6) leaves 500 milliseconds to be read as 500 microseconds with no type name to give it away — both are interval_day. TIME and TIMESTAMP follow via rescaleNanos, and a TIMESTAMP mapped to precisionTimestampTZ is bit-identical to what is emitted, so the semantics flip from instant to local wall-clock silently.

Nothing catches the second tier. VirtualTableScan only validates a virtual table's rows, and the single-argument convert(RexLiteral) used for every predicate, projection, aggregate and window literal (RexExpressionConverter) passes the literal's own type as resultType, so there is no schema to disagree with and no check.

Two things worth settling before the fix, because the first decides how much of this there is to fix.

Whether a UserTypeMapper returning a built-in type is a supported configuration at all. toSubstrait returns any Type, so it is expressible, but toCalcite takes only a Type.UserDefined and TypeConverter.ToRelDataType consults it only from visit(Type.UserDefined) — a mapped built-in therefore has no reverse hook and cannot round-trip. If the answer is that only user-defined types are supported, the fix is to narrow toSubstrait's signature to match its reverse and most of this issue goes away; if built-ins are meant to work, the interface should say so and the reverse needs widening too. #1171 raises the same question from the other end.

The shape of the fix, if the arms are to be fixed. Threading type into each arm closes them one at a time and leaves the next reader unable to tell which arms honour the mapper. Dispatching the whole method on type instead — TypeVisitor.TypeThrowsVisitor is already the idiom for this in TypeConverter.ToRelDataType and FieldReference's finders — makes the mapper honoured by construction and gets the default throw from the base class, at the cost of taking the value from the RexLiteral rather than the type name. The ROW/ARRAY arms need the container's own mapping either way.

Follow-up from the review of #1171. Checked on main @ 5ce7fb9.

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