gabotechs commented on code in PR #16608: URL: https://github.com/apache/datafusion/pull/16608#discussion_r2182113096
########## datafusion/substrait/src/variation_const.rs: ########## @@ -53,6 +53,10 @@ pub const DATE_64_TYPE_VARIATION_REF: u32 = 1; pub const DEFAULT_CONTAINER_TYPE_VARIATION_REF: u32 = 0; pub const LARGE_CONTAINER_TYPE_VARIATION_REF: u32 = 1; pub const VIEW_CONTAINER_TYPE_VARIATION_REF: u32 = 2; +/// Used for the arrow type [`DataType::Map`]. +/// +/// [`DataType::Map`]: datafusion::arrow::datatypes::DataType::Map +pub const DICTIONARY_CONTAINER_TYPE_VARIATION_REF: u32 = 3; Review Comment: 🤔 I see the other `*_CONTAINER_TYPE_VARIATION_REF` are referring to the kind of variations you'd get for the different binary/string/list variants (e.g. `Utf8`, `LargeUtf8` and `Utf8View`) which seem unrelated to dict. I think the fact that the `r#type::Map` type chose to use the `DEFAULT_CONTAINER_TYPE_VARIATION_REF` for their current implementation is an unfortunate choice with no consequences, as the value is `0` anyways. As the `DataType::Dictionary` or `DataType::Map` do **not** have the `DataType::LargeDictionary`, `DataType::DictionaryView`,`DataType::LargeMap` or `DataType::MapView`, I don't think they should be using the `*_CONTAINER_TYPE_VARIATION_REF` variations. What I'd do instead is to have a different set of variation refs for them, something like: ```rust pub const DEFAULT_MAP_TYPE_VARIATION_REF: u32 = 0; pub const DICTIONARY_MAP_TYPE_VARIATION_REF: u32 = 1; ``` what do you think? -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
