alamb commented on code in PR #18086:
URL: https://github.com/apache/datafusion/pull/18086#discussion_r2439222908
##########
datafusion/substrait/src/logical_plan/producer/expr/literal.rs:
##########
@@ -94,6 +95,36 @@ pub(crate) fn to_substrait_literal(
LiteralType::I64(*n as i64),
UNSIGNED_INTEGER_TYPE_VARIATION_REF,
),
+ ScalarValue::Float16(Some(f)) => {
+ // Following rules from
https://github.com/apache/arrow/blame/main/format/substrait/extension_types.yaml
Review Comment:
Perhaps a permalink would be better, something like
https://github.com/apache/arrow/blame/bab558061696ddc1841148d6210424b12923d48e/format/substrait/extension_types.yaml#L23
##########
datafusion/substrait/src/logical_plan/producer/expr/literal.rs:
##########
@@ -94,6 +95,36 @@ pub(crate) fn to_substrait_literal(
LiteralType::I64(*n as i64),
UNSIGNED_INTEGER_TYPE_VARIATION_REF,
),
+ ScalarValue::Float16(Some(f)) => {
+ // Following rules from
https://github.com/apache/arrow/blame/main/format/substrait/extension_types.yaml
+ // fp16 literals are encoded as user defined literals with
+ // a google.protobuf.UInt32Value message where the lower 16 bits
are
+ // the fp16 value.
+ let type_anchor =
producer.register_type(FLOAT_16_TYPE_NAME.to_string());
+
+ // Hmm, the spec says "lower 16 bits" but neglects to mention the
endianness.
+ // Let's just use little-endian for now and update the spec.
Review Comment:
is it worth putting a ticket reference here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
