[GitHub] [arrow] danepitkin commented on a diff in pull request #35210: GH-35209: [Python] Add a type alias for pa.dictionary(pa.int32(), pa.string())

["danepitkin (via GitHub)" <gi...@apache.org>]

danepitkin commented on code in PR #35210:
URL: https://github.com/apache/arrow/pull/35210#discussion_r1170550875


##########
python/pyarrow/tests/test_csv.py:
##########
@@ -952,6 +952,23 @@ def test_decimal_point(self):
             'b': [2.5, -3.0, None],
         }
 
+    def test_dictionary(self):

Review Comment:
   Can you add to the test case `test_schema.py::test_type_for_alias()`? IMO you can delete this test afterwards.



##########
python/pyarrow/types.pxi:
##########
@@ -4790,6 +4790,7 @@ cdef dict _type_aliases = {
     'duration[us]': duration('us'),
     'duration[ns]': duration('ns'),
     'month_day_nano_interval': month_day_nano_interval(),
+    'dictionary[int32,string]': dictionary(int32(), utf8()),

Review Comment:
   ```suggestion
   
       # Composite types
       'dictionary[int32,string]': dictionary(int32(), utf8()),
   ```



##########
python/pyarrow/types.pxi:
##########
@@ -4790,6 +4790,7 @@ cdef dict _type_aliases = {
     'duration[us]': duration('us'),
     'duration[ns]': duration('ns'),
     'month_day_nano_interval': month_day_nano_interval(),
+    'dictionary[int32,string]': dictionary(int32(), utf8()),

Review Comment:
   I wonder if it makes sense to separate composite types from Arrow's primitive types into a different list? Theoretically we could auto generate them for completeness, although I wouldn't want to introduce unnecessary computation on startup. We could lazy-load composite types on usage. 



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org