Minor: rename dictionary_coercion to dictionary_comparison_coercion, add comments#12102
Minor: rename dictionary_coercion to dictionary_comparison_coercion, add comments#12102alamb merged 1 commit intoapache:mainfrom
dictionary_coercion to dictionary_comparison_coercion, add comments#12102Conversation
…n`, add comments
| /// Not all operators support dictionaries, if `preserve_dictionaries` is true | ||
| /// dictionaries will be preserved if possible | ||
| fn dictionary_coercion( | ||
| fn dictionary_comparison_coercion( |
There was a problem hiding this comment.
this is the only code change (and it is not a public function and thus this is not an API change)
| fn dictionary_comparison_coercion( | ||
| lhs_type: &DataType, | ||
| rhs_type: &DataType, | ||
| preserve_dictionaries: bool, |
There was a problem hiding this comment.
I think ideally dictionary_comparison_coercion should not preserve the dictionary (always false), only value type matters. But since it is used in comparison_coercion, the name looks good to me
There was a problem hiding this comment.
In my mind, most single argument functions / operations / coercions would preserve dictionary encoding if possible as that would keep the data encoded in a more efficent form (dictionary) longer
For example substr(dict_column, 3, 10) would ideally produce a Dictionary as output (after applying the function to the values)
However, most of our functions don't know how to work with Dictionary arrays, so this would be a larger change.
|
Thank you for the reviews @jayzhan211 and @andygrove |
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
dictionary_coercionimplies (to me) that this function applies to all dictionary coercions rather than just comparison coercion.What changes are included in this PR?
Changes
dictionary_coerciontodictionary_comparison_coercionAre these changes tested?
Are there any user-facing changes?