Add support for Arrow Dictionary type in Substrait #16608
Conversation
jkosh44
force-pushed
the
substrait-dictionary
branch
2 times, most recently
from
a28e926 to
82327f4
Compare
jkosh44
changed the title
jkosh44
force-pushed
the
substrait-dictionary
branch
from
82327f4 to
dfac1e0
Compare
|
@gabotechs FYI |
| /// Used for the arrow type [`DataType::Map`]. | ||
| /// | ||
| /// [`DataType::Map`]: datafusion::arrow::datatypes::DataType::Map | ||
| pub const DICTIONARY_CONTAINER_TYPE_VARIATION_REF: u32 = 3; |
There was a problem hiding this 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 Maps or Dictionaries.
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:
pub const DEFAULT_MAP_TYPE_VARIATION_REF: u32 = 0;
pub const DICTIONARY_MAP_TYPE_VARIATION_REF: u32 = 1;what do you think?
There was a problem hiding this comment.
Yeah I 100% agree, I've updated it use the consts you've suggested.
This commit adds support for the Arrow Dictionary type in Substrait plans. Resolves apache#16273
jkosh44
force-pushed
the
substrait-dictionary
branch
from
dfac1e0 to
35ff2ed
Compare
* Add support for Arrow Dictionary type in Substrait This commit adds support for the Arrow Dictionary type in Substrait plans. Resolves apache#16273 * Add more specific type variation consts (cherry picked from commit d359d64)
* Add support for Arrow Dictionary type in Substrait (apache#16608) * Add support for Arrow Dictionary type in Substrait This commit adds support for the Arrow Dictionary type in Substrait plans. Resolves apache#16273 * Add more specific type variation consts (cherry picked from commit d359d64) * Remove DataDog Specific Workaround --------- Co-authored-by: Joseph Koshakow <[email protected]>
3 participants
Which issue does this PR close?
Rationale for this change
This commit adds support for the Arrow Dictionary type in Substrait plans.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
Add support for Arrow Dictionary type in Substrait plans