-
Notifications
You must be signed in to change notification settings - Fork 200
Introduce Coral type system abstraction #558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aastha25
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @wmoustafa ! would you also be adding hive type -> coral type converter?
| // Primitive numeric types | ||
| BOOLEAN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for backward compatibility (& a default mapping for unkown types), maybe we should also create NULL / OTHER as it exists today in TypeConverter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know if it is used in practice. Also coral conversion heavily relies on deterministic type inference. We can always add it in the future if the use case arises?
Yes. Probably in a separate PR, but I think this PR is all what we need for #556? |
Oh you meant in that way. I thought that would be part of #556. Let us coordinate how to go about that. |
|
Code changes look good to me. A couple of minor notes that I discovered through i-testing and will be fixing as part of a small follow up PR: (1) and this means Rest, LGTM |
aastha25
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for pushing this through, major milestone for Coral!
| return CharType.of(charType.getLength(), nullable); | ||
| case VOID: | ||
| case UNKNOWN: | ||
| return PrimitiveType.of(CoralTypeKind.STRING, true); // Map to nullable string as a fallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit to use nullable field here instead of hardcoding 'true', but not a blocker.
Summary
Introduces a planner-agnostic Coral type system integrated with Calcite, supporting all major SQL types, nested and parametric types, and precise TIMESTAMP(p) semantics.
Includes a full unit test suite validating nullability, nesting, and precision consistency across primitives and complex types.
Key Aspects
Core Type System
Path:
coral-common/src/main/java/com/linkedin/coral/common/types/Defines:
CoralDataType(base interface) andCoralTypeKindCoralPrimitiveTypeCoralDecimalType(precision, scale)CoralCharType(length)CoralVarcharType(length)CoralArrayType(element type)CoralMapType(key/value types)CoralStructTypeCoralTimestampType(fractional seconds precision 0–9)Establishes Coral’s independent type layer.
Calcite Integration
File:
CoralTypeToRelDataTypeConverter.javaRelDataTypeSTRING→VARCHAR(Hive-style precision)TIMESTAMP(p)correctly (0s, 3ms, 6µs)Testing
File:
CoralTypeSystemTest.javaCovers:
DECIMAL(precision/scale)CHAR/VARCHAR(length)ARRAY/MAP(element nullability)STRUCT(simple + nested)TIMESTAMPprecision mapping (0, 3, 6)